Summary: fix slowness due to layout thrashing when reloading a large … #12661
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
hold off on merging this: I've introduced a bug when viewing a status thread where if the text is too long it's hidden by a too-small div |
|
Please also check code style errors found by CodeClimate! |
panarom
force-pushed
the
issue-12455—slow-back-button
branch
from
58f21c4
to
ee1abd6
Compare
…set of status updates in order to limit the maximum size of a status in a list view (e.g. the home timeline), so as to avoid having to scroll all the way through an abnormally large status update (see mastodon#8205), the following steps are taken: •the element containing the status is rendered in the browser •its height is calculated, to determine if it exceeds the maximum height threshold. Unfortunately for performance, these steps are carried out in the componentDidMount(/Update) method, which also performs style modifications on the element. The combination of height request and style modification during javascript evaluation in the browser leads to layout-thrashing, where the elements are repeatedly re-laid-out (see https://developers.google.com/web/fundamentals/performance/rendering/avoid-large-complex-layouts-and-layout-thrashing & https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Performance_best_practices_for_Firefox_fe_engineers). The solution implemented here is to memoize the collapsed state in Redux the first time the status is seen (e.g. when fetched as part of a small batch, to populate the home timeline) , so that on subsequent re-renders, the value can be queried, rather than recalculated. This strategy is derived from mastodon#4439 & mastodon#4909, and should resolve mastodon#12455. Andrew Lin (https://github.com/onethreeseven) is thanked for his assistance in root cause analysis and solution brainstorming
…t is restored e.g. when navigating from home-timeline to a status conversational thread and <Back again
panarom
force-pushed
the
issue-12455—slow-back-button
branch
from
b1e88da
to
c5f8fd5
Compare
Gargron
approved these changes
Dec 29, 2019
zunda
added a commit
to zunda/mastodon
that referenced
this pull request
Dec 30, 2019
…a large … (mastodon#12661)" This reverts commit 31f7c3f.
Gargron
added a commit
that referenced
this pull request
Dec 30, 2019
zunda
added a commit
to zunda/mastodon
that referenced
this pull request
Dec 30, 2019
…loading a large … (mastodon#12661)"" This reverts commit 1e7b5fb.
Gargron
added a commit
that referenced
this pull request
Dec 30, 2019
panarom
added a commit
to panarom/mastodon
that referenced
this pull request
Jan 2, 2020
…lableList is restored" This reverts commit 07e2614. accidentally merged spurious code in mastodon#12661. mastodon#12735 removes the slowdown that this code was trying to solve; and other functionality successfully restores the view state of the list
panarom
added a commit
to panarom/mastodon
that referenced
this pull request
Jan 2, 2020
… identical value" This reverts commit c93df21. accidentally merged spurious code in mastodon#12661. mastodon#12735 removes the slowdown that this code was trying to solve; and other functionality successfully restores the view state of the list
Merged
Gargron
pushed a commit
that referenced
this pull request
Jan 2, 2020
* Revert "persist last-intersected status update and restore when ScrollableList is restored" This reverts commit 07e2614. accidentally merged spurious code in #12661. #12735 removes the slowdown that this code was trying to solve; and other functionality successfully restores the view state of the list * Revert "cache currently-viewing status id to avoid calling redux with identical value" This reverts commit c93df21. accidentally merged spurious code in #12661. #12735 removes the slowdown that this code was trying to solve; and other functionality successfully restores the view state of the list
|
My instance is on 3.1 now, and it does seem better! Thanks! |
|
This reintroduces a lot of issues with the TL moving on its own when a status is deleted etc. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…set of status updates
in order to limit the maximum size of a status in a list view (e.g. the home timeline), so as to avoid having to scroll all the way through an abnormally large status update (see #8205), the following steps are taken:
•the element containing the status is rendered in the browser
•its height is calculated, to determine if it exceeds the maximum height threshold.
Unfortunately for performance, these steps are carried out in the componentDidMount(/Update) method, which also performs style modifications on the element. The combination of height request and style modification during javascript evaluation in the browser leads to layout-thrashing, where the elements are repeatedly re-laid-out (see https://developers.google.com/web/fundamentals/performance/rendering/avoid-large-complex-layouts-and-layout-thrashing & https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Performance_best_practices_for_Firefox_fe_engineers).
The solution implemented here is to memoize the collapsed state in Redux the first time the status is seen (e.g. when fetched as part of a small batch, to populate the home timeline) , so that on subsequent re-renders, the value can be queried, rather than recalculated. This strategy is derived from #4439 & #4909, and should resolve #12455.
Andrew Lin (https://github.com/onethreeseven) is thanked for his assistance in root cause analysis and solution brainstorming