-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
useSelect: only invalidate on subscribe if store changed #57108
Conversation
Size Change: +646 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Flaky tests detected in 211dbd3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7226521084
|
const subscribe = ( _listener ) => { | ||
cleanup(); | ||
return subscriber.subscribe( _listener ); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the returned subscribe
function is never called, the subscription you’re newly creating will leak, if will be there forever and never cleaned up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix in the next commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsnajdr Ok, I changed it to avoid subscribing and simply compare the store states with those on mount. If all the states are equal, then lastMapResultValid
can remain true.
storeStatesOnMount[ index ] === | ||
// To do: figure out why it's sometimes not available in | ||
// unit tests. | ||
registry.stores[ storeName ]?.store?.getState?.() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsnajdr @youknowriad Any idea why this is not available in some unit tests? Something about custom generic stores? Is there a better way to may store names to their state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, custom generic stores. The things in registry.stores
have an interface that doesn't guarantee a .store
property, only the "mainstream" Redux stores have that. The rest have only the published selectors.
I'm curious if this state comparison is still a performance optimization, as it's a completely different approach than the original subscribe
one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be, the graph looks the same as the other optimisation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting -6.75% for first block load in this run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if there's no interface, we can always return false for now (previous behaviour). But strange that there's no way to get the state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we could add an optional getSnapshot
or getStableReference
to the interface to address this use-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we try that separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second run after the rewrite is 6.6% for first block load
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we could add an optional
getSnapshot
orgetStableReference
to the interface
Yes, and to prevent misuse to read private-ish data, return an empty object {}
memoized/keyed in weakmap by the real state object.
activeStores.length === storeStatesOnMount.length && | ||
activeStores.every( | ||
( storeName, index ) => | ||
storeStatesOnMount[ index ] === |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subscribe
function could be called multiple times to obtain multiple subscriptions. React probably never does it, but it's a reasonable operation anyway. Here, the second subscribe
call will dereference a null
object and crash.
By the way, what if storeStatesOnMount
was an object keyed by store name? Seems better than array to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's true, a Map is cleaner and also solves the other issue.
storeStatesOnMount[ index ] === | ||
// To do: figure out why it's sometimes not available in | ||
// unit tests. | ||
registry.stores[ storeName ]?.store?.getState?.() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we could add an optional
getSnapshot
orgetStableReference
to the interface
Yes, and to prevent misuse to read private-ish data, return an empty object {}
memoized/keyed in weakmap by the real state object.
What?
Currently we invalidate the selector mapping result whenever React subscribes to the store. This is to make sure the value is fresh because it might have changed between the first time we update the value and when React subscribes.
Instead, temporarily listen for store changes and only invalidate the result if it changed (which shouldn't happen often).
Why?
This prevents all the
useSelect
result from being calculated twice on load/mount.First run load first block -6.75% (after the rewrite)
Second run -6.6%
The block marked red is now gone:
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast