Redux store: batch resolver startResolution actions #60590
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +981 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e985deb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8773898925
|
ellatrix
changed the title
jsnajdr
reviewed
Apr 10, 2024
| @@ -570,6 +571,8 @@ function mapResolvers( resolvers ) { | |||
| } ); | |||
| } | |||
|
|
|||
| const queue = []; | |||
There was a problem hiding this comment.
There should be a separate queue per registry. Otherwise, if there are multiple registries resolving selectors, cross-registry batching will be done, like this:
registryOne.batch( () => {
registryTwo.dispatch( queuedAction );
} );and that doesn't make sense. Won't crash, but no batching is really done.
| } ); | ||
|
|
||
| // Many resolvers can be called at once. The point of this is to at | ||
| // least batch `startResolution` actions all together. |
There was a problem hiding this comment.
The queue will batch not only the initial startResolution actions, but also the first actions executed inside resolver.fulfill (the resolver actions are usually thunks). In other words, everything until the first await is batched.
I'm afraid this might be too sloppy -- we're batching arbitrary user-space actions without their consent. Expect that, somewhere in the 6.6 beta cycle, someone will come to report that they have a store subscriber that expects store changes with certain granularity, one per action, but now they are all swallowed in a batch and only one big update comes at the end.
There was a problem hiding this comment.
Why wouldn't you want the actions to be batched? Does it matter?
There was a problem hiding this comment.
Batching pauses all store listeners while the batch is processed. In fact this is the only thing that it does. And that changes what the store observers see. Instead of seeing some value go from 0 to 1 and then back to 0, it will be flat 0. Instead of seeing thing A change in one update and then thing B change in another update, you'll see A and B both change in one update.
All I'm saying is that user of the library will notice this, it will break someone's code and they will come here to complain 🙂
There was a problem hiding this comment.
Ok let's only batch our actions then 👍
There was a problem hiding this comment.
Ok, I separated these actions and changed it to only batch startResolution actions together.
jsnajdr
approved these changes
Apr 12, 2024
|
|
||
| queue.push( [ startResolution, fulfillResolution ] ); | ||
|
|
||
| window.queueMicrotask( () => { |
There was a problem hiding this comment.
btw this code should be able to run also in raw Node.js, without any DOM APIs. Promise.resolve().then( ... ) is a universal way to queue a microtask.
There was a problem hiding this comment.
Also, when starting 100 resolutions at once, there will be 100 microtasks queued of which 99 will see an empty queue. Could we queue just one microtask?
There was a problem hiding this comment.
How do we know when to queue that one microtask? We have some similar techniques across the codebase, but I haven't seen one where only one task is queued, and not sure how it'd be possible?
There was a problem hiding this comment.
Something like this should work:
let queue = [];
function queue( task ) {
queue.push( task );
if ( queue.length === 1 ) {
scheduleTimer( () => {
queue.forEach( ... );
queue = [];
} );
}
}One subtle question is whether the queue can be reentrant, i.e., can the code run inside queue.forEach add new tasks to the queue? In our case, can it happen that inside resolver.fulfill another resolver starts to be fulfilled?
There was a problem hiding this comment.
Then it would push a task to the queue and will still be done?
There was a problem hiding this comment.
Depends on how exactly you loop through the queue array. Your for ( const fulfill of queue ) will see new items added to queue inside the loop. But queue.forEach won't, it will process only items that were in the queue when the .forEach was called.
ellatrix
force-pushed
the
try/batch-resolvers
branch
from
21925dd
to
e985deb
Compare
|
@jsnajdr Ok, it seems like this is causing trouble with the new approach: https://github.com/WordPress/gutenberg/pull/43480/files#diff-fec6390e24332dec4a0c7c74f9f7b9a015ce191e0e6348361ead793ceda3991dR298 |
|
I restored it back to the previous version be that leaves us again with the other issue #60590 (comment) So I guess we'll have to close this since it can't be done 😞 |
|
Let's close, I don't see a solution here |
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.
What?
Lots of resolver selectors get instantly fulfilled. We should batch them if we can.
Why?
I expect the biggest result to come for the initial site editor load (first block):
-2.53% first run
-1.14% second run
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast