-
-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use composition to improve tab switch #231
Conversation
Thanks for making a pull request to lumino! |
0b1f92d
to
b9dff75
Compare
@jasongrout would you mind having a second look at this one? |
@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. |
Our current CSS makes this hidden class an |
this._items.length > 1 && | ||
this._hiddenMode === Widget.HiddenMode.Scale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
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.
|
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.
Thanks for the review @jasongrout |
CI errors are related to broken link in README |
I submitted another PR to your branch with a few more follow-up changes: fcollonval#2 |
packages/widgets/src/docklayout.ts
Outdated
// 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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll udpate the PR to not change the hidden mode of the widget being removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@jasongrout is this PR ready from your point of view? |
Yes, thanks! |
Thanks a lot @jasongrout for the PRs and reviews. |
Optionally use composition style for high speed tab switch