useSelect: don't subscribe at all if out of view #32091
Closed
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
ellatrix
added
[Type] Performance
|
Size Change: -114 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
try/use-select-out-of-view-unsubscribe
branch
from
75eecca to
5a9ccf1
Compare
ellatrix
force-pushed
the
try/use-select-out-of-view-unsubscribe
branch
from
5a9ccf1 to
0379f18
Compare
ellatrix
force-pushed
the
try/use-select-out-of-view-unsubscribe
branch
from
0379f18 to
c9c9e24
Compare
ellatrix
commented
Oct 3, 2022
| @@ -942,7 +942,7 @@ describe( 'useSelect', () => { | |||
| expect( rendered.getByRole( 'status' ) ).toHaveTextContent( 1 ); | |||
|
|
|||
| // initial render + subscription check + rerender with isAsync=false | |||
| expect( selectSpy ).toHaveBeenCalledTimes( 3 ); | |||
| expect( selectSpy ).toHaveBeenCalledTimes( 4 ); | |||
There was a problem hiding this comment.
This happens because the useIsomorphicLayoutEffect dependency isAsync changes, which seems normal. Comment needs updating.
There was a problem hiding this comment.
If the isSyncing = isAsync != null from https://github.com/WordPress/gutenberg/pull/32091/files#r986891973 is implemented, then this test need not change. There will be no new mapSelect call.
ellatrix
changed the title
ellatrix
commented
Oct 3, 2022
| @@ -207,7 +207,7 @@ export default function useSelect( mapSelect, deps ) { | |||
| const isMounted = useRef( false ); | |||
|
|
|||
| useIsomorphicLayoutEffect( () => { | |||
| if ( ! hasMappingFunction ) { | |||
| if ( ! hasMappingFunction || isAsync === null ) { | |||
There was a problem hiding this comment.
Either we create a new possible value null here, meaning "don't subscribe and update", or we need a separate context provider.
There was a problem hiding this comment.
So, in addition to an "async" mode, we now also have a "nosync" mode 🙂 it's turned on when mounting the provider as <AsyncModeProvider value={ null } /> which is not particularly intuitive but anyway.
One unwanted consequence of having isAsync as a dependency of this effect is that it will run (and resubscribe) also when the value changes between false and true. We don't want that, the subscription should stay the same. We only want to react to changes between null and true | false. Let's do something like:
const isSyncing = isAsync != null;
useEffect( () => {
if ( ! isSyncing ) {
return;
}
// subscription mgmt stuff
}, [ isSyncing, /* ... */ ] );Another place that needs updating is the hasLeftAsyncMode condition a few lines above. Newly we want to force running the mapSelect function also when isAsync changes from null to true | false. Otherwise it will render the stale latestMapOutput value that was computed either on first render or just before the component went to nosync mode.
We want to ensure (and also unit test) that merely rerendering AsyncModeProvider with a changed value prop will cause the children to read the latest data store state. In current Gutenberg the value change is probably always associated with data store state change (I'd guess blockVisibility?), so the observable UI behavior is good, but under different circumstances it could break.
ellatrix
force-pushed
the
try/use-select-out-of-view-unsubscribe
branch
from
6bce173 to
6bd9134
Compare
ellatrix
commented
Oct 3, 2022
| @@ -32,10 +33,6 @@ export function useInBetweenInserter() { | |||
| const { showInsertionPoint, hideInsertionPoint } = | |||
| useDispatch( blockEditorStore ); | |||
|
|
|||
| const delayedShowInsertionPoint = useDebounce( showInsertionPoint, 500, { | |||
| trailing: true, | |||
| } ); | |||
There was a problem hiding this comment.
I reverted all of #44325, but we can probably keep this bit. Only the IntersectionObserver debouncing needs to be reverted I think.
|
Turning off all data updates for off-screen blocks is a radical move, and can be risky. Here's one example where an off-screen block influences visible UI. The Home template in the Twenty Twenty-Two theme contains two patterns, the header image and the footer content. In List View, they are initially shown as Pattern blocks:
Only after the pattern selector gets resolved (
If the Pattern block is off-screen, the data store update after the pattern is loaded from REST API will not be triggered. The block remains a Pattern until it's scrolled into view. |
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.
Description
Proposal: only update blocks that are either selected or in view. Not needing to update them means we don't need to subscribe to the store at all. This eliminates a huge amount of work to be done. There's absolutely no reason to keep subscriptions to the store for blocks that are not visible and not selected.
It's worth noting that the first block render is still done, but it still improves load performance significantly. This is for accessibility so it's still possible to navigate the blocks. If a block is outdated and becomes selected, it will instantly update.
Typing performance seems to be more than cut in half.
This should also reduce energy consumption and improve battery life when using the editor.
How has this been tested?
Load the large post and type.
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).