Reduce rerendering on startup #12847
Conversation
|
Tests seem to be failing due to a configuration issue with |
packages/core-data/src/selectors.js
Outdated
| @@ -178,5 +178,5 @@ export function isPreviewEmbedFallback( state, url ) { | |||
| * @return {boolean} Upload Permissions. | |||
| */ | |||
| export function hasUploadPermissions( state ) { | |||
| return state.hasUploadPermissions; | |||
| return !! state.hasUploadPermissions; | |||
There was a problem hiding this comment.
What's the reason for this change? This should be something where, if we're expecting it to be a boolean, it should be forced to be a boolean in the reducer logic, not derived by the selector.
There was a problem hiding this comment.
I was reluctant to make the change in the reducer, as it seemed more prone to create unintended side-effects, in case something is relying on the "objectness" of this part of the state tree. I'm happy to shift the change there if that's what you feel is best, though.
There was a problem hiding this comment.
Force-pushed this change. This is now handled at the reducer level where we default to true instead of {}. I chose true instead of false, since {} is a truthy value, and this should thus maintain existing behaviour.
There was a problem hiding this comment.
I'm guessing the previous implementation was a result of some bad copypasta from other reducers. A reducer value prefixed with "has" should almost certainly hold a boolean value.
Were there unit tests which should be updated as part of the change? Can there be, if not? 😄
There was a problem hiding this comment.
Nope, this particular reducer didn't seem to have any unit tests. I added a couple :)
sgomes
force-pushed
the
perf/reduce-rerendering-on-startup
branch
3 times, most recently
from
e7ae814 to
0e88f4e
Compare
Could you elaborate on why this change was made? I mean, it seems like a perfectly correct fix, just not sure how it relates to the performance work. |
|
I might expect it having something to do with the fact that state "changed" previously (from |
That's exactly why, yes, sorry I wasn't very clear in the issue. The commit message had more details; I'll copy them over to the issue for reference. |
|
@aduth Thanks for the link to #12877! That's a lot better than any startup optimisations opportunities I've found so far, which tended to have a nice large impact in development mode, but next to nothing in production (I assume this has to do with the time it takes to do the extra logging that takes place in development). Also, thanks for adding the changelog commits! |
We need to call `flush`, so that the changes happen synchronously, rather than asynchronously, on the next event loop. This avoids the re-rendering of all `BlockListBlock` instances during startup.
The reducer for this property defaults to an empty object, which not only is incorrect (since this is a boolean), but also causes unnecessary re-renders by changing reference. By defaulting to `true` instead, we fix both problems.
youknowriad
force-pushed
the
perf/reduce-rerendering-on-startup
branch
from
943b054 to
b44bc8f
Compare
* Ensure viewport data store changes happen immediately on startup. We need to call `flush`, so that the changes happen synchronously, rather than asynchronously, on the next event loop. This avoids the re-rendering of all `BlockListBlock` instances during startup. * Ensure `state.hasUploadPermissions` is always a boolean. The reducer for this property defaults to an empty object, which not only is incorrect (since this is a boolean), but also causes unnecessary re-renders by changing reference. By defaulting to `true` instead, we fix both problems. * Add a couple of tests for hasUploadPermissions reducer. * Core Data: Add CHANGELOG entry for hasUplaodPermissions boolean fix * Viewport: Add CHANGELOG entry for synchronous assignment
Description
Small tweaks that result in less unnecessary work during startup. This is a very modest speed improvement (after the changes, the time spent in JS during startup goes down by about 3-5%), but it doesn't introduce much additional complexity at just 2 lines of code.
How has this been tested?
Unit tests, ad-hoc testing. Changes are minor and easy to analyse.
For performance measuring, I tried to create as stable an environment as possible on my local machine (with as few applications running as possible), throttled the CPU by 6x, and ran startup profiles for a large post with many blocks, repeatedly. For each of these runs, I measured how much time the sum of the two long frames during startup took.
Types of changes
Ensure viewport calculations happen synchronously instead of waiting on event loop.
We need to call
flush, so that the changes happen synchronously, rather than asynchronously, on the next event loop. This avoids the re-rendering of allBlockListBlockinstances during startup.Ensure
state.hasUploadPermissionsis always a boolean.The reducer for this property defaults to an empty object, which not only is incorrect (since this is a boolean), but also causes unnecessary re-renders by changing reference. By defaulting to
trueinstead, we fix both problems.Checklist:
refs #11782