Avoid triggering sync layout on web #521
Conversation
Reading container.offsetLeft, container.Top etc. triggers layout recalculation and unnecessarily stalls JS execution. Fixed in this commit by always subscribing to _reportLayoutChange and pushing our logic down to the deferred callback.
Asking for viewport dimensions triggers layout recalculation and unnecessarily stalls JS execution. Scrollbars having the exact correct size on first render is arguably not worth the perf hit (much less so when they are initially hidden which is the common case). Also deferring _tryLtrOverride() for the same reason. It's not clear when the neutral override class would actually be used. Maybe this whole thing can be removed? (Apologies, my web-fu is weak.)
Setting tabIndex to -1 on an element that's active triggers layout recalculation and unnecessarily stalls JS execution. Defer this work to the next timer tick.
|
I don't see a way to work around _recalcPosition. We need that information before we can display the popup in its intended position. Deferring the work will not help the perceived performance and will likely hurt it. I'm concerned about the negative perf impact of this change — in particular, the change in ViewBase. This change will cause a timer to be initialized for every update of a view that has an onLayout handler specified. The previous code tried very hard to avoid this if it wasn't necessary (i.e. the view didn't change size or position). |
| this._asyncInitTimer = window.setTimeout(() => { | ||
| this._tryLtrOverride(); | ||
| this.update(); | ||
| }, 0); |
There was a problem hiding this comment.
You should set this._asyncInitTimer to undefined within the callback. Otherwise clearInterval will be called on an already-fired timer in dispose.
src/web/ViewBase.tsx
Outdated
| return SyncTasks.Resolved<void>(); | ||
| } | ||
| const deferred = SyncTasks.Defer<void>(); | ||
| ViewBase._reportLayoutChange(() => { |
There was a problem hiding this comment.
I worry about the perf implications of this change. The previous code tried very hard to avoid creating timers in the frequent case where the view size and position didn't change. The new code always allocates a timer on every update. Have you done sufficient perf measurements on parts of the code that will be impacted by this (e.g. the VMV in Skype)?
There was a problem hiding this comment.
I see. I wonder if there's an easy way to use the new behavior only if we know that it's going to help. Maybe only if a popup is being rendered for the first time. I'll report back shortly, thanks!
| // Defer the work to avoid triggering sync layout. | ||
| FocusManager._resetFocusTimer = setTimeout(() => { | ||
| const prevTabIndex = FocusManager._setTabIndex(document.body, 0); | ||
| document.body.focus(); |
There was a problem hiding this comment.
Please have Marat review this to make sure it's not introducing an undesirable race condition. For example, what if the user presses the tab key to move the focus to the next item? This key event used to arrive after the synchronous code ran, but now it can arrive before the deferred code runs.
- reset CustomScrollbar._asyncInitTimer to undefined when it fires - made the deferred work in FocusManager idempotent
…ange fires" This reverts commit ef5e701.
Checking layout after showing a popup triggers layout recalculation and unnecessarily stalls JS execution. Avoid that by deferring this work under these specific circumstances.
|
I have reverted the first commit and instead made ViewBase defer the entire _checkAndReportLayout() if a popup was just shown. This solves the unwanted sync layout problem with popups without incurring any cost to the common path. |
Got it. One layout should be fine. The problem with the current code is that layout is triggered repeatedly and the browser is doing throw-away work just to satisfy our reads. Thanks! |
* Sync layout: Don't touch view container until _reportLayoutChange fires Reading container.offsetLeft, container.Top etc. triggers layout recalculation and unnecessarily stalls JS execution. Fixed in this commit by always subscribing to _reportLayoutChange and pushing our logic down to the deferred callback. * Sync layout: Defer initial CustomScrollbar sizing Asking for viewport dimensions triggers layout recalculation and unnecessarily stalls JS execution. Scrollbars having the exact correct size on first render is arguably not worth the perf hit (much less so when they are initially hidden which is the common case). Also deferring _tryLtrOverride() for the same reason. It's not clear when the neutral override class would actually be used. Maybe this whole thing can be removed? (Apologies, my web-fu is weak.) * Sync layout: Defer problematic tabIndex changes Setting tabIndex to -1 on an element that's active triggers layout recalculation and unnecessarily stalls JS execution. Defer this work to the next timer tick. * Addressed feedback microsoft#1 - reset CustomScrollbar._asyncInitTimer to undefined when it fires - made the deferred work in FocusManager idempotent * Revert "Sync layout: Don't touch view container until _reportLayoutChange fires" This reverts commit ef5e701. * Sync layout: Defer _checkAndReportLayout after showing a popup Checking layout after showing a popup triggers layout recalculation and unnecessarily stalls JS execution. Avoid that by deferring this work under these specific circumstances.
2 participants
Displaying a popup triggers synchronous layout (aka "recalculate style", "forced reflow") at multiple places. This is a collection of tweaks to avoid some of these and make popups faster to appear.
Time spent in layout went down from ~220 ms to ~150 ms when showing the emoticon picker in S4L Electron. This translated to the popup showing onClick handler taking ~80 ms (or ~20%) less time overall.
The remaining offender, not addressed in this PR, is RootView._recalcPosition() which heavily depends on measuring various elements. I'm open to suggestions on how tackle this one. As noted in the commit messages, my web-fu is weak :)