Minimize the calls in useSelect by subscribing to only the stores needed #26724
Conversation
|
Size Change: +617 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
|
@kevin940726 it doesn't account for selectors like export const getWidgets = createRegistrySelector( ( select ) => () => {
const widgets = select( 'core' ).getEntityRecords(
'root',
'widget',
buildWidgetsQuery()
);
return keyBy( widgets, 'id' );
} );To make it work, you'd need to somehow hook into the actual |
|
@kevin940726 I played with it more and came up with this: #26733 - still not working properly though |
gziolo
added
[Package] Data
kevin940726
force-pushed
the
try/use-select-performance
branch
2 times, most recently
from
6866559 to
048c33b
Compare
|
Does it support this as well #26724 (comment) ? |
|
@youknowriad Yep, it's also in the unit tests :). |
kevin940726
force-pushed
the
try/use-select-performance
branch
from
0b6b5d2 to
5c099ce
Compare
|
@kevin940726 I think this PR is obsolete now that #26866 was merged |
Thx for the notice! Note to myself that I'll try to run some of the unit tests introduced here after rebasing and close this 👍. |
|
Actually, I'm considering reverting the |
kevin940726
force-pushed
the
try/use-select-performance
branch
from
5c099ce to
0ce8271
Compare
|
@youknowriad Thx for the inputs! I've rebased the branch and updated the performance report in the description. |
|
It seems the performance impact here is good but not as good as what we had in the atom's approach. |
Our stores are large so it's normal to have selectors "watching" a branch that remains the same over multiple "notifyListeners" calls. That's another difference with the atoms approach where only relevant changes bubble up to their observers. |
Description
Minimize the calls of the selectors in
useSelectby only subscribing to the stores needed.The problem
Currently,
useSelectworks by subscribing to theregistryinstance and calls themapSelect(the first argument ofuseSelect) function every time the store changes. It works fine, but it also fires many unnecessarymapSelectcalls when we know that the store updates won't cause a re-render.Most components subscribe to the default
registry, and most stores also registered to that sameregistry. Let's say that some component has acounterstore and multiple other stores likecore,core/editor,core/block-editor, etc. Thecounterstore has a selectorgetCounter, which simply gets thecountstate from the store. A component using this selector could look like this.It's clear that this component doesn't care about state updates from stores other than
counter. However, the selector still gets called when we dispatch any actions no matter if the action doesn't belong to thecounterstore.Fortunately, since the
countstate fromgetCounter()doesn't change,<TestComponent>doesn't have to re-render. Still, it's an unnecessary call of thegetCounterselector. In this specific case, it doesn't matter much, but it would matter if themapSelectfunction is very complex or slow. Especially when we haveresolverfor eachselector, which could potentially cause unwanted side-effects to be introduced.The solution
This PR propose a solution to minimize the calls of the
mapSelectfunction as much as possible. The idea is to only call themapSelectfunction if and only if the store thismapSelecthas referenced to gets updated.For instance,
this
useSelectonly cares about updates within thecounterstore itself and nothing else. We only have to re-run themapSelectfunction whenever the state from thecounterstore updates.Another example,
this
useSelectcares about state updates from two different stores:counterandcore/editor. Whenever either one of the store updates, we need to re-run themapSelectfunction.This is done by trapping the
selectfunction of theregistryto keep track of every store it has called. It works very similarly like monkey-patching theselectfunction as follow (the actual implementation is slightly different because of some issues with legacy features withuseandwithPlugins):Whenever
reigstry.select( storeKey )is called, we'll push thestoreKeyto thelisteningStores. Later, we can then only subscribe to those stores rather than subscribing to the wholeregistry.How has this been tested?
Added a bunch of unit tests and also fixed some tests.
End-to-end tests should also be passing.
Performance tests
As seen in the report, this PR out-performs every test. However, the difference is not that big though, it might be just a micro-optimization in the end.
Types of changes
New feature
Checklist: