Skip to content

[Sidebar][BlockPatterns] Prevent pattern previews from rendering in parallel due to React's concurrent mode #48085

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

Closed
fullofcaffeine wants to merge 1 commit into from
Closed

[Sidebar][BlockPatterns] Prevent pattern previews from rendering in parallel due to React's concurrent mode #48085

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion packages/compose/src/hooks/use-async-list/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ function useAsyncList< T >(
...state,
...list.slice( nextIndex, nextIndex + step ),
] );
asyncQueue.add( {}, append( nextIndex + step ) );

queueMicrotask( () =>
asyncQueue.add( {}, append( nextIndex + step ) )
);
Comment on lines +69 to +71
Copy link
Member

@tyxla tyxla Feb 15, 2023

Choose a reason for hiding this comment

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

This may have an unexpected side effect on other areas of the editor that use the useAsyncList hook. Have you measured any potential effects there?

I wonder if alternatively, we can just debounce the showing of the pattern previews instead of the addition of patterns to the async list?

Copy link
Contributor

@youknowriad youknowriad Feb 15, 2023

Choose a reason for hiding this comment

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

I wonder if alternatively, we can just debounce the showing of the pattern previews instead of the addition of patterns to the async list?

That's the whole purpose of "useAsyncList" debounce the showing of the previews. But instead of relying on random "debounce" value, it relies on requestIdleCallback internally (in priority queue) to continue rendering previews as long as the browser is idle.

Copy link
Member

@tyxla tyxla Feb 15, 2023

Choose a reason for hiding this comment

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

Makes sense to me.

Do you have any concerns with shipping this change @youknowriad? I wasn't able to measure any noticeable difference before and after this PR in my testing.

Copy link
Contributor

@youknowriad youknowriad Feb 15, 2023

Choose a reason for hiding this comment

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

The change is fine but I'm surprised it's needed. Maybe @fullofcaffeine know the difference here

Copy link
Member

@dmsnell dmsnell Feb 15, 2023

Choose a reason for hiding this comment

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

We have a bug that's fixed by changing a subtle detail deep inside the core and there's no comment. Seems like a very good place to say "We cannot use asyncQueue.add() here because …" 😉

Copy link
Member

@dmsnell dmsnell Feb 16, 2023

Choose a reason for hiding this comment

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

class PriorityQueue {
	add( queue, callback ) {
		callback();
	}
}

🤡

Copy link
Member

@jsnajdr jsnajdr Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When originally adapting useAsyncList for concurrent mode, I didn't notice that we're checking timeRemaining() and trying to fit as much work as possible into one idle callback. I thought it's just a series of idle callbacks each performing step renders. Exactly the behavior we're getting in this branch after adding the queueMicrotasks.

Here's how we could get the original behavior even in concurrent mode:

async function renderStep( nextIndex ) {
  // schedules setState, including the rendering, for next microtask tick
  ReactDOM.flushSync( () => {
    setCurrent( state => state.concat( list.slice( nextIndex, nextIndex + step ) ) );
  } );
  // schedules a microtask that finishes _after_ the render is done
  await Promise.resolve();

  return nextIndex + step;
}

async function runWaitingList( deadline ) {
  while ( nextIndex < list.length ) {
    // add new items to the list, render, and wait until the rendering finishes
    nextIndex = await renderStep( nextIndex );

    // if we still have time, continue rendering in this idle callback
    if ( deadline.timeRemaining() > 0 ) {
      continue;
    }

    // otherwise schedule a new idle callback, with a new deadline, and
    // continue rendering after it fires.
    deadline = await new Promise( ( resolve ) => {
      requestIdleCallback( ( d ) => resolve( d ) );
    } );
  }
}

requestIdleCallback( runWaitingList );

The magical trick here is that the idle callback can be an async function, doing its work asynchronously, with promises, and still keep checking the deadline!. It doesn't need to be fully sync.

The code I wrote doesn't use the priority-queue, it calls requestIdleCallback directly.

To make it work with priority-queue, we'd have to:

  1. Make the runWaitingList function async, and make it await callback().
  2. Add all the work to the queue at once, we don't need the tail recursion for asyncQueue.add that we have now. We know upfront what we want to get done, and then it will get executed in idle callbacks.
for ( let i = 0; i < list.length; i += step ) {
  const nextList = list.slice( 0, i + step );
  asyncQueue.add( {}, async () => {
    ReactDOM.flushSync( () => setCurrent( nextList ) );
    await Promise.resolve();
  } );
}

One thing about flushSync is that it's in the react-dom package. For mobile, we'd have to use the react-native equivalent, which I hope exists. And export them from @wordpress/element.

By the way, I think that in concurrent mode we eventually shouldn't need useAsyncList at all. Processing a long render queue asynchronously, taking smaller bites, checking deadlines, and letting higher-priority interruptions run, that's what concurrent mode is supposed to do natively. I don't have much insight into the precise details right now, but once we learn about that, we should be able to eliminate useAsyncList.

Copy link
Contributor

@youknowriad youknowriad Feb 20, 2023

Choose a reason for hiding this comment

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

Ok so ReactDOM.flushSync doesn't re-render synchronously? So in that case we still need the microtask. In that case, yeah the only part forward would be to promisify priority-queue (or built a promisified alternative). Also, I don't see why we need flushSync in this case? what does it do? setCurrent already schedules the microtask internally

Copy link
Contributor

@youknowriad youknowriad Feb 20, 2023

Choose a reason for hiding this comment

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

By the way, I think that in concurrent mode we eventually shouldn't need useAsyncList at all. Processing a long render queue asynchronously, taking smaller bites, checking deadlines, and letting higher-priority interruptions run, that's what concurrent mode is supposed to do natively. I don't have much insight into the precise details right now, but once we learn about that, we should be able to eliminate useAsyncList.

Yeah, I agree but it seems React is y et to provide the APIs to define the priorities and basically useAsyncList could probably but one of these APIs.

Copy link
Member

@jsnajdr jsnajdr Feb 20, 2023

Choose a reason for hiding this comment

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

Ok so ReactDOM.flushSync doesn't re-render synchronously?

Turns out I was wrong abou this: flushSync is indeed completely synchronous, including effects. That means we don't need any microtasks and promises, only a flushSync wrapper around setCurrent. I submitted this fix as #48238.

Also, I don't see why we need flushSync in this case? what does it do? setCurrent already schedules the microtask internally

I'm not 100% sure if the scheduling of setState is always the microtask. The scheduling strategy is different in different scenarios, and sometimes the update can be scheduled for later.

};
asyncQueue.add( {}, append( firstItems.length ) );

Expand Down