Skip to content

feat: Cache status height to avoid expensive renders#4439

Merged
Gargron merged 4 commits intomastodon:masterfrom
sorin-davidoi:cache-status-height
Aug 7, 2017
Merged

feat: Cache status height to avoid expensive renders#4439
Gargron merged 4 commits intomastodon:masterfrom
sorin-davidoi:cache-status-height

Conversation

@sorin-davidoi
Copy link
Copy Markdown
Contributor

Scenario:

  • Scroll down the federated timeline (say that there are 200 toots, but only 30 are fully rendered)
  • You find an interesting toot and click on it, being taken to the details status
  • You click back to go to the federated timeline
  • Although the scroll position is restored, we render all the 200 toots instead of just the 30 required - this can take a few seconds

The reason that we render all 200 toots is that their rendered height was lost when they were unmounted (we have to fully render them again to save the original height).

This pull request saves the height of the toots in the store, so that we only fully render the 30 toots. This "cached height" is invalidated if the window is resized.

@sorin-davidoi sorin-davidoi added the performance Runtime performance label Jul 29, 2017
@sorin-davidoi
Copy link
Copy Markdown
Contributor Author

Would be nice if this could be deployed on a test instance to make sure nothing breaks.

@beatrix-bitrot
Copy link
Copy Markdown
Contributor

@sorin-davidoi aye aye

@sorin-davidoi sorin-davidoi added work in progress Not to be merged, currently being worked on and removed work in progress Not to be merged, currently being worked on labels Jul 29, 2017
@Gargron
Copy link
Copy Markdown
Member

Gargron commented Jul 29, 2017

Has this now been confirmed to not break stuff?

@sorin-davidoi
Copy link
Copy Markdown
Contributor Author

Not yet. If this will receive extra testing I've also moved the formatting logic into the reducers (no need to call escapeTextContentForBrowser and emojify on the same input at each render).

@sorin-davidoi
Copy link
Copy Markdown
Contributor Author

sorin-davidoi commented Aug 1, 2017

This is what I'm talking about:

image

We render toots only to immediately destroy them. This makes the experience laggy and if the device runs out of memory the browser may actually kill the tab.

overflow-y: scroll;
overflow-x: hidden;
flex: 1 1 auto;
backface-visibility: hidden;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creates redundant layers that the browser has to keep in the GPU memory.

import { Map as ImmutableMap, fromJS } from 'immutable';
import escapeTextContentForBrowser from 'escape-html';

const domParser = new DOMParser();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

@sorin-davidoi
Copy link
Copy Markdown
Contributor Author

Think this is enough for this PR. @beatrix-bitrot maybe you can take this for a test pretty please?

@nightpool
Copy link
Copy Markdown
Member

What's the status of the PR? Can we merge it or do we still want to test it first?

@sorin-davidoi
Copy link
Copy Markdown
Contributor Author

I'd still want some testing first.

@clworld
Copy link
Copy Markdown
Contributor

clworld commented Aug 7, 2017

@sorin-davidoi I deployed this on my staging server and does some test.
It seems work without glitch. It's OK as I see.

@Gargron Gargron merged commit 8eb6d17 into mastodon:master Aug 7, 2017
ykzts added a commit that referenced this pull request Aug 7, 2017
Gargron pushed a commit that referenced this pull request Aug 7, 2017
* Revert "Adjust tags and accounts page (#4534)"

This reverts commit a3e53bd.

* Revert "feat: Cache status height to avoid expensive renders (#4439)"

This reverts commit 8eb6d17.

* Revert "Refactor Avatar and AvatarOverlay to have 'account' as prop instead of src and staticSrc (#4526)"

This reverts commit 5942347.

* Revert "Update dependencies for Ruby (#4543)"

This reverts commit 22db947.

* Revert "[Docker] Add multicore support to "make" and "bundler" (#4544)"

This reverts commit 5d408fd.

* Revert "It makes no sense to try using invalid or expired link again (#4521)"

This reverts commit 47579ec.

* Revert "i18n: Update Polish translation (#4545)"

This reverts commit 3363a05.

* Revert "Set false to animated options for thumbnail processor (#4547)"

This reverts commit 87f10d4.
@abcang abcang mentioned this pull request Sep 12, 2017
panarom added a commit to panarom/mastodon that referenced this pull request Dec 28, 2019
…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
Gargron pushed a commit that referenced this pull request Dec 29, 2019
#12661)

* Summary: fix slowness due to layout thrashing when reloading a large 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

* remove getSnapshotBeforeUpdate from status

* remove componentWillUnmount from status

* persist last-intersected status update and restore when ScrollableList is restored

e.g. when navigating from home-timeline to a status conversational  thread and <Back again

* cache currently-viewing status id to avoid calling redux with identical value

* refactor collapse toggle to pass explicit boolean
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Runtime performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants