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

Focus file before modifying DOM to avoid forced reflows #58

Merged
merged 5 commits into from Feb 8, 2018

Conversation

Projects
None yet
3 participants
@50Wliu
Member

50Wliu commented Dec 8, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Focusing an element is considered a "read" operation. Since we were mutating the DOM before selecting the element, the focus call was causing a double forced reflow. By waiting until after the focus call to mutate the DOM, we avoid those forced reflows, which were taking ~220ms combined. While reflows still have to occur, they seem to be more efficient than the forced reflows as seen below. I've also set contain: strict on the Archive View to improve layout performance.

Before After
1484ms forced-reflows 1200ms regular-reflow

Alternate Designs

Don't focus a file on creation. That however is not very user-friendly.

Benefits

Removes a source of forced reflows and minorly improves the time it takes until the Archive View is usable.

Possible Drawbacks

If an exception occurs while creating the tree entries or updating the summary, the loading view will not be removed. Similarly with selecting the first file and actually adding the entries to the DOM.

Applicable Issues

Refs #19

50Wliu added some commits Dec 8, 2017

@50Wliu

This comment has been minimized.

Member

50Wliu commented Dec 8, 2017

@simurai do you know anything about the contain property? Reading MDN it seems like I can use contain: strict here and everything seems to work but I'd like to be sure that it won't horribly crash and burn in some edge cases.

@simurai

This comment has been minimized.

Member

simurai commented Dec 8, 2017

I haven't experimented with contain yet. But since there is an overflow: auto; I think it's ok to use. There shouldn't be anything that can affect something outside of the overflow.

/cc @as-cii that has used contain for the editor.

@as-cii

This comment has been minimized.

Member

as-cii commented Dec 11, 2017

I haven't tested this but it would seem okay to use this approach. A couple of questions/comments:

  • Forced reflows are not bad per se, unless they cause "layout thrashing" (e.g. read, write, read, write, etc.), which didn't seem to occur in the CPU profile you posted. Did the performance gain come from using stronger layout containment?
  • I am unclear about the drawbacks of this approach and how they compare to the benefits this pull-request is providing. Could an exception occur while creating tree entries? How did we handle this situation before the proposed changes?

Thanks, @50Wliu!

@50Wliu

This comment has been minimized.

Member

50Wliu commented Dec 11, 2017

@as-cii yeah I agree that the forced reflows aren't really bad in this case because they happen anyways later on, but even before applying the updated containment rules the profiling was consistently showing 100-200ms improvements after fixing the reflows.

Could an exception occur while creating tree entries?

I'm highly skeptical that this could occur (as mostly all it does is append elements to the DOM), but I forgot to mention that even if it does happen a Notification will be generated. There was no special handling beforehand either, but now we hide the loading message after the tree entries have finished being generated rather than before (which also makes more sense logically).

@as-cii

This comment has been minimized.

Member

as-cii commented Feb 7, 2018

Okay this seems good to go. Thanks for your patience on this, @50Wliu! 🚢 at will.

@50Wliu 50Wliu merged commit 3adba18 into master Feb 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the wl-avoid-forced-reflow branch Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment