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.

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

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

Conversation

fullofcaffeine
Copy link
Member

@fullofcaffeine fullofcaffeine commented Feb 15, 2023

Fixes: #48084

What?

Fixes an issue that causes Gutenberg to stop handling UI events and sometimes crashing the browser due to the new concurrent mode implemented and the rendering of too many block pattern preview iframes in the sidebar inserter after searching.

Why?

After concurrent mode was activated in #46467, in some scenarios, the sidebar would block the entire Gutenberg UI because the browser is trying to render too many documents inside the block pattern preview iframes in parallel, effectively blocking the event loop and blocking the UI, and sometimes even crashing the browser in some systems (see issue).

How?

Use queueMicroTask (though setTimeout with a timeout of 0 also worked) to schedule the asynceQueue add call to the next tick. This seems to bring the behavior of block pattern previews closer to how it was before React concurrent mode was implemented, when they were rendered sequentially/with a slight delay, giving the browser time to render the expensive iframes without blocking the UI when there are many instances of them (see the issue for more info).

Testing Instructions

First, follow the steps to reproduce the issue, then:

  1. Clone Gutenberg locally from this branch, build, and then install/activate it in your WP instance;
  2. Open the inserter and search for the term that matches 50-60 patterns. The issue might happen with fewer patterns being rendered, but it might also be system/browser-dependent. To make sure you have the environment closest to the one I used to reproduce it, try to have at least 50 registered block patterns that have actual previews in them. An easy way to do that is to use a Wordpress.com test site, if you have access to one.
  3. Immediately after you finish typing the search term in the sidebar inserter's search box, notice how it updates the block and pattern contents below. Try to move the mouse over immediately. The pattern preview buttons should be loaded sequentially, without causing the UI to block. You should be able to see the UI changes reflect without the UI freezing. Try clicking, hovering and scrolling, the UI should work well.

Testing Instructions for Keyboard

Screenshots or screencast

Before the fix:

bad.mp4

After the fix:

good.mp4

…ent pattern previews to be rendered in parallel
@fullofcaffeine fullofcaffeine force-pushed the try/improve-rendering-perf-concurrent-mode-pattern-preview branch from 918f511 to 344c8bc Compare February 15, 2023 01:48
@github-actions
Copy link

Flaky tests detected in 344c8bc.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4179793081
📝 Reported issues:

@tyxla tyxla requested a review from youknowriad February 15, 2023 10:24
Comment on lines +69 to +71
queueMicrotask( () =>
asyncQueue.add( {}, append( nextIndex + step ) )
);
Copy link
Member

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

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

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

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

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

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

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

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

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

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.

@tyxla
Copy link
Member

tyxla commented Feb 15, 2023

On a side note, performance tests appear to be failing consistently. I've rerun them again, let's see what happens.

@WunderBart
Copy link
Member

On a side note, performance tests appear to be failing consistently. I've rerun them again, let's see what happens.

These failures seem to be unrelated, also happening on other branches. I've started investigating and hopefully should have a fixing PR open soon!

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Feb 16, 2023
@tyxla
Copy link
Member

tyxla commented Feb 20, 2023

@fullofcaffeine it seems like we could close this one in favor of #48238.

@fullofcaffeine
Copy link
Member Author

fullofcaffeine commented Feb 21, 2023

Closing this in favor of #48238. Thanks @jsnajdr!

@youknowriad youknowriad deleted the try/improve-rendering-perf-concurrent-mode-pattern-preview branch February 21, 2023 17:57
@ntsekouras ntsekouras removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React concurrent mode can cause UI rendering to block in the sidebar inserter due to pattern previews
7 participants