Use composition to improve tab switch #231

fcollonval · 2021-09-14T13:31:46Z

Optionally use composition style for high speed tab switch

jupyterlab-probot · 2021-09-14T13:31:49Z

Thanks for making a pull request to lumino!
To try out this branch on binder, follow this link:

packages/widgets/src/widget.ts

fcollonval · 2021-09-30T09:13:09Z

@jasongrout would you mind having a second look at this one?

jasongrout · 2021-10-09T23:50:10Z

@fcollonval - I added some documentation and code style suggestions in fcollonval#1.

I have a few other comments I'll add here for discussion as well.

jasongrout · 2021-10-09T23:55:54Z

The current way (referred to by Class in this PR) has the nice effect to let the developer choosing the way it will deal with hiding the widget as you mentioned. So although changing the display style is the default rule applied, it could be overridden.

Our current CSS makes this hidden class an !important rule, so yes, technically they can override, but then they'd have to also use an important rule, which is awkward. I think it's easier to just say that this display:none CSS rule wasn't designed to be overridden.

jasongrout · 2021-10-09T04:09:10Z

packages/widgets/src/stackedlayout.ts

+      this._items.length > 1 &&
+      this._hiddenMode === Widget.HiddenMode.Scale


Can you switch the order of these conditions? It reads a little nicer if the more important condition is first. Plus most often the length will be more than one and the mode will not be scale, so we might as well get the shortcut evaluation from the && more often.

packages/widgets/src/stackedlayout.ts

packages/widgets/src/docklayout.ts

jasongrout · 2021-10-10T00:49:03Z

Overall, I'm curious - where did you see the display method being a bottleneck, where the scaling method gives us better performance? I'm curious what kinds of performance gains you are seeing.

fcollonval · 2021-10-12T09:02:12Z

where did you see the display method being a bottleneck, where the scaling method gives us better performance? I'm curious what kinds of performance gains you are seeing.

This came when looking at that issue on tab switching. The trouble with large notebook is the style computation - using the notebook in the provided link, the current reported action on Chrome is:

Most of the time is spent in style recalculation.

When using the transform trick scale(0), the actions are:

The rendering action in the above test is decreased from ~250ms to ~100ms.

From tests on Windows and Linux, this is very efficient on Chrome but not so much on Firefox.

jasongrout · 2021-10-12T09:09:42Z

The rendering action in the above test is decreased from ~250ms to ~100ms.

Nice!! Thanks for documenting it here too.

* Descriptions should be full sentences. * Clarify that if there is one widget, what mode will actually be used. * Wrap lines to ~80 columns * Document both getters and setters for consistency with other methods * Add what the default is in options. * Notes are level-4 headers for consistency.

…ode. Also, for consistency between these two functions, set the private variable before handling ramifications.

fcollonval · 2021-10-13T12:39:46Z

Thanks for the review @jasongrout
I applied the suggestions in 29114e6.

fcollonval · 2021-10-13T15:39:41Z

CI errors are related to broken link in README https://mybinder.org/v2/gh/jupyterlab/lumino/master?urlpath=lab/tree/examples. It is not related to this PR.

jasongrout · 2021-10-14T20:54:17Z

I submitted another PR to your branch with a few more follow-up changes: fcollonval#2

jasongrout · 2021-10-14T21:04:00Z

packages/widgets/src/docklayout.ts

+    // We don't switch back the hidden mode of the widget to avoid triggering
+    // style recomputation
+    if (this._hiddenMode === Widget.HiddenMode.Scale) {
+      widget.node.style.willChange = 'auto';


Can you explain a bit more this logic around setting the node.style.willChange attribute here and just below this, and in particular why we don't want to set the hidden mode directly, and let these style.willChange attributes change at the widget level now? I think that's the only part about this PR I don't understand.

If this line is changed to widget.hiddenMode = Widget.HiddenMode.Display, it will be more consistent. But if the widget is hidden, that will result in the classes on the widget node to change (addition of lm-mod-hidden). This will trigger style recomputation as the widget is not yet detached that could take some times to resolve. So this was the reason not to switch back the hidden mode. But it brings indeed unconsistency. The best would be to reset the hidden mode once we are sure the widget is detached. Do you have any suggestion to improve that part?

Yeah, this does seem a bit tricky. Presumably the browser optimizes things if the widget is currently hidden?

Have we profiled this optimization of not using scaled when there is one widget, and in practice how much that helps?

Presumably the browser optimizes things if the widget is currently hidden?

Definitely. Not changing anything may indeed be the best approach.

Have we profiled this optimization of not using scaled when there is one widget, and in practice how much that helps?

Pete (Colab) commented that creating too many layers will have a negative performance impact. I try the composition change with 20 big notebooks opened simultaneously and did not see major impact (unfortunately the negative impact is probably related to the user hardware).
To limit the risk, I went for not creating unneeded layer if the stack panel was only containing one widget.

I'll udpate the PR to not change the hidden mode of the widget being removed.

Done in 8cb3932

We did a similar change in docklayout in 29114e6. This also changes the logic to an early-return pattern for consistency with other code in Lumino.

…layout The previous code assumes the widget hidden mode is display. This makes sure it is when it is added to the container.

In 29114e6, this logic was moved to the widget class.

…k layout This is more consistent with resetting other attributes of the removed widget. It also makes the hidden mode for a single remaining widget slightly less complicated, as that first widget will now be shown and *then* its hidden mode changed (thus having no immediate effect), rather than changing the hidden mode (triggering an immediate change), *then* showing it.

fcollonval · 2021-10-27T13:37:11Z

@jasongrout is this PR ready from your point of view?

jasongrout · 2021-11-04T16:58:44Z

@jasongrout is this PR ready from your point of view?

Yes, thanks!

fcollonval · 2021-11-05T07:26:07Z

Thanks a lot @jasongrout for the PRs and reviews.

fcollonval mentioned this pull request Sep 14, 2021

Use transform to quickly switch between tabs. jupyterlab/jupyterlab#11074

Merged

jasongrout reviewed Sep 14, 2021

View reviewed changes

packages/widgets/src/widget.ts Outdated Show resolved Hide resolved

blink1073 mentioned this pull request Sep 15, 2021

Class and aria attribute must be changed on title #232

Merged

fcollonval requested a review from jasongrout September 22, 2021 06:58

fcollonval added 5 commits September 30, 2021 11:05

Use composition to improve tab switch

3d9e625

Allow to switch hiddenMode a posteriori

4212eb6

Rename enum

6bb08b7

Add tests and improve documentation

1da9dd4

Lint the code

b9dff75

fcollonval force-pushed the ft/hide-compositino branch from 0b1f92d to b9dff75 Compare September 30, 2021 09:09

jasongrout reviewed Oct 10, 2021

View reviewed changes

jasongrout and others added 3 commits October 13, 2021 13:40

Do an early return if setting is a no-op for consistency with other c…

e1f6bdc

…ode. Also, for consistency between these two functions, set the private variable before handling ramifications.

Apply review suggestions

29114e6

Lint files

e222200

jasongrout mentioned this pull request Oct 14, 2021

A few more followup commits to https://github.com/jupyterlab/lumino/pull/231 fcollonval/lumino#2

Merged

jasongrout reviewed Oct 14, 2021

View reviewed changes

jasongrout added 3 commits October 15, 2021 11:46

Do not set style willChange attributes in stackedlayout.

6d3df4e

We did a similar change in docklayout in 29114e6. This also changes the logic to an early-return pattern for consistency with other code in Lumino.

Set the hiddenMode for widgets that are added to docklayout and stack…

15ae9c6

…layout The previous code assumes the widget hidden mode is display. This makes sure it is when it is added to the container.

Do not change the widget hidden mode styling in stackedlayout.

418e206

In 29114e6, this logic was moved to the widget class.

jasongrout and others added 4 commits October 15, 2021 11:46

Fix vocabulary in tests from old hidden modes to current terminology.

922b0f5

Lint code

ea4db6a

no hidden mode change when removing from dockpanel

8cb3932

mlucool mentioned this pull request Oct 27, 2021

Proposal: Jupyter should be able to handle large notebooks jupyterlab/benchmarks#75

Open

blink1073 added the enhancement New feature or request label Oct 29, 2021

jasongrout merged commit bffb4d7 into jupyterlab:master Nov 4, 2021

fcollonval deleted the ft/hide-compositino branch November 5, 2021 07:25

krassowski mentioned this pull request Dec 19, 2021

How to benchmark lumino branches/forks? jupyterlab/benchmarks#83

Open

krassowski mentioned this pull request Oct 5, 2022

Switching to a big notebook in a tab is slow jupyterlab/jupyterlab#4292

Open

github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use composition to improve tab switch #231

Use composition to improve tab switch #231

fcollonval commented Sep 14, 2021

jupyterlab-probot bot commented Sep 14, 2021

fcollonval commented Sep 30, 2021

jasongrout commented Oct 9, 2021

jasongrout commented Oct 9, 2021

jasongrout Oct 9, 2021

jasongrout commented Oct 10, 2021 •

edited

Loading

fcollonval commented Oct 12, 2021

jasongrout commented Oct 12, 2021

fcollonval commented Oct 13, 2021

fcollonval commented Oct 13, 2021

jasongrout commented Oct 14, 2021

jasongrout Oct 14, 2021 •

edited

Loading

fcollonval Oct 15, 2021

jasongrout Oct 15, 2021

fcollonval Oct 16, 2021

fcollonval Oct 16, 2021

fcollonval Oct 27, 2021

fcollonval commented Oct 27, 2021

jasongrout commented Nov 4, 2021

fcollonval commented Nov 5, 2021

		this._items.length > 1 &&
		this._hiddenMode === Widget.HiddenMode.Scale

Use composition to improve tab switch #231

Use composition to improve tab switch #231

Conversation

fcollonval commented Sep 14, 2021

jupyterlab-probot bot commented Sep 14, 2021

fcollonval commented Sep 30, 2021

jasongrout commented Oct 9, 2021

jasongrout commented Oct 9, 2021

jasongrout Oct 9, 2021

Choose a reason for hiding this comment

jasongrout commented Oct 10, 2021 • edited Loading

fcollonval commented Oct 12, 2021

jasongrout commented Oct 12, 2021

fcollonval commented Oct 13, 2021

fcollonval commented Oct 13, 2021

jasongrout commented Oct 14, 2021

jasongrout Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

fcollonval Oct 15, 2021

Choose a reason for hiding this comment

jasongrout Oct 15, 2021

Choose a reason for hiding this comment

fcollonval Oct 16, 2021

Choose a reason for hiding this comment

fcollonval Oct 16, 2021

Choose a reason for hiding this comment

fcollonval Oct 27, 2021

Choose a reason for hiding this comment

fcollonval commented Oct 27, 2021

jasongrout commented Nov 4, 2021

fcollonval commented Nov 5, 2021

jasongrout commented Oct 10, 2021 •

edited

Loading

jasongrout Oct 14, 2021 •

edited

Loading