Skip to content
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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Dec 15, 2023
Merged

useSelect: only invalidate on subscribe if store changed #57108

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1f26294
WIP
ellatrix Dec 15, 2023
6147977
Mark dirty only if store changed
ellatrix Dec 15, 2023
ba4764e
Fix unit tests
ellatrix Dec 15, 2023
a94d977
Rewrite
ellatrix Dec 15, 2023
1631033
Clarify, simplify, and fix tests
ellatrix Dec 15, 2023
211dbd3
Address feedback: use Map and adjust comments
ellatrix Dec 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion packages/data/src/components/use-select/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ function Store( registry, suspense ) {
let lastIsAsync;
let subscriber;
let didWarnUnstableReference;
let storeStatesOnMount;

const createSubscriber = ( stores ) => {
// The set of stores the `subscribe` function is supposed to subscribe to. Here it is
Expand All @@ -61,7 +62,16 @@ function Store( registry, suspense ) {
// in the interval between the `getValue` call during render and creating
// the subscription, which is slightly delayed. We need to ensure that this
// second `getValue` call will compute a fresh value.
lastMapResultValid = false;
lastMapResultValid =
activeStores.length === storeStatesOnMount.length &&
activeStores.every(
( storeName, index ) =>
storeStatesOnMount[ index ] ===
Copy link
Member

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.

Copy link
Member Author

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.

// To do: figure out why it's sometimes not available in
// unit tests.
registry.stores[ storeName ]?.store?.getState?.()
Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with it.

Copy link
Member Author

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

Copy link
Member

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

Yes, and to prevent misuse to read private-ish data, return an empty object {} memoized/keyed in weakmap by the real state object.

);
storeStatesOnMount = null;

const onStoreChange = () => {
// Invalidate the value on store update, so that a fresh value is computed.
Expand Down Expand Up @@ -149,6 +159,9 @@ function Store( registry, suspense ) {
}

if ( ! subscriber ) {
storeStatesOnMount = listeningStores.current.map( ( name ) =>
registry.stores[ name ]?.store?.getState?.()
);
subscriber = createSubscriber( listeningStores.current );
} else {
subscriber.updateStores( listeningStores.current );
Expand Down
82 changes: 41 additions & 41 deletions packages/data/src/components/use-select/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe( 'useSelect', () => {
// 2 selectSpy calls expected
// - 1 for initial mount
// - 1 for the subscription effect checking if value has changed
expect( selectSpy ).toHaveBeenCalledTimes( 2 );
expect( selectSpy ).toHaveBeenCalledTimes( 1 );
expect( TestComponent ).toHaveBeenCalledTimes( 1 );

// Ensure expected state was rendered.
Expand Down Expand Up @@ -93,7 +93,7 @@ describe( 'useSelect', () => {
</RegistryProvider>
);

expect( selectSpyFoo ).toHaveBeenCalledTimes( 2 );
expect( selectSpyFoo ).toHaveBeenCalledTimes( 1 );
expect( selectSpyBar ).toHaveBeenCalledTimes( 0 );
expect( TestComponent ).toHaveBeenCalledTimes( 1 );

Expand All @@ -107,7 +107,7 @@ describe( 'useSelect', () => {
</RegistryProvider>
);

expect( selectSpyFoo ).toHaveBeenCalledTimes( 2 );
expect( selectSpyFoo ).toHaveBeenCalledTimes( 1 );
expect( selectSpyBar ).toHaveBeenCalledTimes( 0 );
expect( TestComponent ).toHaveBeenCalledTimes( 2 );

Expand All @@ -121,7 +121,7 @@ describe( 'useSelect', () => {
</RegistryProvider>
);

expect( selectSpyFoo ).toHaveBeenCalledTimes( 2 );
expect( selectSpyFoo ).toHaveBeenCalledTimes( 1 );
expect( selectSpyBar ).toHaveBeenCalledTimes( 1 );
expect( TestComponent ).toHaveBeenCalledTimes( 3 );

Expand Down Expand Up @@ -163,7 +163,7 @@ describe( 'useSelect', () => {

// Initial render renders only parent and subscribes the parent to store.
expect( screen.getByText( 'none' ) ).toBeInTheDocument();
expect( mapSelectParent ).toHaveBeenCalledTimes( 2 );
expect( mapSelectParent ).toHaveBeenCalledTimes( 1 );
expect( mapSelectChild ).toHaveBeenCalledTimes( 0 );
expect( Parent ).toHaveBeenCalledTimes( 1 );
expect( Child ).toHaveBeenCalledTimes( 0 );
Expand All @@ -174,8 +174,8 @@ describe( 'useSelect', () => {

// Child was rendered and subscribed to the store, as the _second_ subscription.
expect( screen.getByText( 'yes' ) ).toBeInTheDocument();
expect( mapSelectParent ).toHaveBeenCalledTimes( 3 );
expect( mapSelectChild ).toHaveBeenCalledTimes( 2 );
expect( mapSelectParent ).toHaveBeenCalledTimes( 2 );
expect( mapSelectChild ).toHaveBeenCalledTimes( 1 );
expect( Parent ).toHaveBeenCalledTimes( 2 );
expect( Child ).toHaveBeenCalledTimes( 1 );

Expand All @@ -187,8 +187,8 @@ describe( 'useSelect', () => {
// I.e., `mapSelectChild` was called again, and state update was scheduled, we cannot
// avoid that, but the state update is never executed and doesn't do a rerender.
expect( screen.getByText( 'none' ) ).toBeInTheDocument();
expect( mapSelectParent ).toHaveBeenCalledTimes( 4 );
expect( mapSelectChild ).toHaveBeenCalledTimes( 3 );
expect( mapSelectParent ).toHaveBeenCalledTimes( 3 );
expect( mapSelectChild ).toHaveBeenCalledTimes( 2 );
expect( Parent ).toHaveBeenCalledTimes( 3 );
expect( Child ).toHaveBeenCalledTimes( 1 );
} );
Expand Down Expand Up @@ -217,7 +217,7 @@ describe( 'useSelect', () => {
</RegistryProvider>
);

expect( mapSelect ).toHaveBeenCalledTimes( 2 );
expect( mapSelect ).toHaveBeenCalledTimes( 1 );
expect( TestComponent ).toHaveBeenCalledTimes( 1 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0:0' );

Expand All @@ -226,7 +226,7 @@ describe( 'useSelect', () => {
registry.dispatch( 'store-even' ).inc();
} );

expect( mapSelect ).toHaveBeenCalledTimes( 3 );
expect( mapSelect ).toHaveBeenCalledTimes( 2 );
expect( TestComponent ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0:2' );

Expand All @@ -235,7 +235,7 @@ describe( 'useSelect', () => {
registry.dispatch( 'store-odd' ).inc();
} );

expect( mapSelect ).toHaveBeenCalledTimes( 3 );
expect( mapSelect ).toHaveBeenCalledTimes( 2 );
expect( TestComponent ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0:2' );

Expand All @@ -244,7 +244,7 @@ describe( 'useSelect', () => {
registry.dispatch( 'store-main' ).inc();
} );

expect( mapSelect ).toHaveBeenCalledTimes( 4 );
expect( mapSelect ).toHaveBeenCalledTimes( 3 );
expect( TestComponent ).toHaveBeenCalledTimes( 3 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '1:3' );

Expand All @@ -253,7 +253,7 @@ describe( 'useSelect', () => {
registry.dispatch( 'store-odd' ).inc();
} );

expect( mapSelect ).toHaveBeenCalledTimes( 5 );
expect( mapSelect ).toHaveBeenCalledTimes( 4 );
expect( TestComponent ).toHaveBeenCalledTimes( 4 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '1:5' );

Expand All @@ -263,7 +263,7 @@ describe( 'useSelect', () => {
registry.dispatch( 'store-even' ).inc();
} );

expect( mapSelect ).toHaveBeenCalledTimes( 6 );
expect( mapSelect ).toHaveBeenCalledTimes( 5 );
expect( TestComponent ).toHaveBeenCalledTimes( 4 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '1:5' );
} );
Expand Down Expand Up @@ -336,7 +336,7 @@ describe( 'useSelect', () => {
expect( screen.getByRole( 'status' ).dataset.d ).toBe(
JSON.stringify( valueB )
);
expect( mapSelectSpy ).toHaveBeenCalledTimes( 3 );
expect( mapSelectSpy ).toHaveBeenCalledTimes( 2 );
}
);
} );
Expand Down Expand Up @@ -368,26 +368,26 @@ describe( 'useSelect', () => {
</RegistryProvider>
);

expect( selectCount1 ).toHaveBeenCalledTimes( 2 );
expect( selectCount2 ).toHaveBeenCalledTimes( 2 );
expect( selectCount1 ).toHaveBeenCalledTimes( 1 );
expect( selectCount2 ).toHaveBeenCalledTimes( 1 );
expect( TestComponent ).toHaveBeenCalledTimes( 1 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0' );

act( () => {
registry.dispatch( 'store-2' ).inc();
} );

expect( selectCount1 ).toHaveBeenCalledTimes( 2 );
expect( selectCount2 ).toHaveBeenCalledTimes( 3 );
expect( selectCount1 ).toHaveBeenCalledTimes( 1 );
expect( selectCount2 ).toHaveBeenCalledTimes( 2 );
expect( TestComponent ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0' );

act( () => {
registry.dispatch( 'store-1' ).inc();
} );

expect( selectCount1 ).toHaveBeenCalledTimes( 3 );
expect( selectCount2 ).toHaveBeenCalledTimes( 3 );
expect( selectCount1 ).toHaveBeenCalledTimes( 2 );
expect( selectCount2 ).toHaveBeenCalledTimes( 2 );
expect( TestComponent ).toHaveBeenCalledTimes( 3 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '1' );

Expand Down Expand Up @@ -425,21 +425,21 @@ describe( 'useSelect', () => {
</RegistryProvider>
);

expect( selectCount1And2 ).toHaveBeenCalledTimes( 2 );
expect( selectCount1And2 ).toHaveBeenCalledTimes( 1 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0,0' );

act( () => {
registry.dispatch( 'store-2' ).inc();
} );

expect( selectCount1And2 ).toHaveBeenCalledTimes( 3 );
expect( selectCount1And2 ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0,1' );

act( () => {
registry.dispatch( 'store-3' ).inc();
} );

expect( selectCount1And2 ).toHaveBeenCalledTimes( 3 );
expect( selectCount1And2 ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0,1' );
} );

Expand Down Expand Up @@ -475,7 +475,7 @@ describe( 'useSelect', () => {
</RegistryProvider>
);

expect( selectCount1AndDep ).toHaveBeenCalledTimes( 2 );
expect( selectCount1AndDep ).toHaveBeenCalledTimes( 1 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count:0,dep:0'
);
Expand All @@ -484,7 +484,7 @@ describe( 'useSelect', () => {
setDep( 1 );
} );

expect( selectCount1AndDep ).toHaveBeenCalledTimes( 3 );
expect( selectCount1AndDep ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count:0,dep:1'
);
Expand All @@ -493,7 +493,7 @@ describe( 'useSelect', () => {
registry.dispatch( 'store-1' ).inc();
} );

expect( selectCount1AndDep ).toHaveBeenCalledTimes( 4 );
expect( selectCount1AndDep ).toHaveBeenCalledTimes( 3 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count:1,dep:1'
);
Expand Down Expand Up @@ -636,7 +636,7 @@ describe( 'useSelect', () => {
</RegistryProvider>
);

expect( selectCount1And2 ).toHaveBeenCalledTimes( 2 );
expect( selectCount1And2 ).toHaveBeenCalledTimes( 1 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count1:0,count2:0'
);
Expand All @@ -645,7 +645,7 @@ describe( 'useSelect', () => {
registry.dispatch( 'store-2' ).inc();
} );

expect( selectCount1And2 ).toHaveBeenCalledTimes( 3 );
expect( selectCount1And2 ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count1:0,count2:1'
);
Expand Down Expand Up @@ -691,15 +691,15 @@ describe( 'useSelect', () => {
);

expect( selectCount1 ).toHaveBeenCalledTimes( 0 );
expect( selectCount2 ).toHaveBeenCalledTimes( 2 );
expect( selectCount2 ).toHaveBeenCalledTimes( 1 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count2:0'
);

act( () => screen.getByText( 'Toggle' ).click() );

expect( selectCount1 ).toHaveBeenCalledTimes( 1 );
expect( selectCount2 ).toHaveBeenCalledTimes( 2 );
expect( selectCount2 ).toHaveBeenCalledTimes( 1 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count1:0'
);
Expand All @@ -710,7 +710,7 @@ describe( 'useSelect', () => {
} );

expect( selectCount1 ).toHaveBeenCalledTimes( 2 );
expect( selectCount2 ).toHaveBeenCalledTimes( 2 );
expect( selectCount2 ).toHaveBeenCalledTimes( 1 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count1:1'
);
Expand Down Expand Up @@ -968,7 +968,7 @@ describe( 'useSelect', () => {
);

// initial render + missed update catcher in subscribing effect
expect( selectSpy ).toHaveBeenCalledTimes( 2 );
expect( selectSpy ).toHaveBeenCalledTimes( 1 );
expect( TestComponent ).toHaveBeenCalledTimes( 1 );

// Ensure expected state was rendered.
Expand All @@ -979,12 +979,12 @@ describe( 'useSelect', () => {
} );

// still not called right after increment
expect( selectSpy ).toHaveBeenCalledTimes( 2 );
expect( selectSpy ).toHaveBeenCalledTimes( 1 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0' );

expect( await screen.findByText( 1 ) ).toBeInTheDocument();

expect( selectSpy ).toHaveBeenCalledTimes( 3 );
expect( selectSpy ).toHaveBeenCalledTimes( 2 );
expect( TestComponent ).toHaveBeenCalledTimes( 2 );
} );

Expand Down Expand Up @@ -1027,7 +1027,7 @@ describe( 'useSelect', () => {
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '1' );

// initial render + subscription check + rerender with isAsync=false
expect( selectSpy ).toHaveBeenCalledTimes( 3 );
expect( selectSpy ).toHaveBeenCalledTimes( 2 );
// initial render + rerender with isAsync=false
expect( TestComponent ).toHaveBeenCalledTimes( 2 );
} );
Expand Down Expand Up @@ -1077,8 +1077,8 @@ describe( 'useSelect', () => {
// Give the async update time to run in case it wasn't cancelled
await new Promise( setImmediate );

expect( selectA ).toHaveBeenCalledTimes( 2 );
expect( selectB ).toHaveBeenCalledTimes( 2 );
expect( selectA ).toHaveBeenCalledTimes( 1 );
expect( selectB ).toHaveBeenCalledTimes( 1 );
expect( TestComponent ).toHaveBeenCalledTimes( 2 );
} );

Expand Down Expand Up @@ -1120,7 +1120,7 @@ describe( 'useSelect', () => {
await new Promise( setImmediate );

// only the initial render, no state updates
expect( selectSpy ).toHaveBeenCalledTimes( 2 );
expect( selectSpy ).toHaveBeenCalledTimes( 1 );
expect( TestComponent ).toHaveBeenCalledTimes( 1 );
} );

Expand Down Expand Up @@ -1163,7 +1163,7 @@ describe( 'useSelect', () => {
await new Promise( setImmediate );

// initial render + registry change rerender, no state updates
expect( selectSpy ).toHaveBeenCalledTimes( 4 );
expect( selectSpy ).toHaveBeenCalledTimes( 2 );
expect( TestComponent ).toHaveBeenCalledTimes( 2 );
} );
} );
Expand Down
Loading
Loading