Async mode in the data module #13056

youknowriad · 2018-12-21T11:17:53Z

This is a crazy experiment :)

The idea is to build an async mode into the data module. Allowing some part of the React Tree to declare them selves as "async" which basically means low priority.

If a component is in async mode, it won't react synchronously to store changes, instead it will wait for all the browser's high-priority work (typing, rendering...)

In its current state, this PR declares the whole block list (the content of the post) as low priority, which means this part won't rerender synchronously when you type for instance but the chrome (save buttons, sidebar...) will rerender in sync.

I think this may introduce some small bugs we need to track and see how to fix (slot/fill maybe others).

That said, the feeling of responsiveness is highly improved with this PR. For instance you can try with this post https://gist.github.com/florianbrinkmann/4e9e4d23eaefb8b23484badddd848196 (convert it to blocks first) and compare the feeling with master.

Refs #11782

gziolo · 2018-12-22T10:23:23Z

I think this proposal makes a lot of sense. In particular, the way this async mode is introduced seems to be reasonable (as far as I understood these changes). I read it as this async mode is applied to all unselected blocks when there is no multi-selection mode enabled.

I guess that the failing test/e2e/specs/multi-block-selection.test.js test might be a legitimate one given that we introduce the change when the check for multi-selection is applied to the blocks list component.

This makes me think, whether we should also explore the opposite approach at some point, make everything async by default and mark as sync only those branches of virtual DOM as sync where it is essential to provide a compelling UX.

By the way, communication in React Native between JavaScript and native code is async by design, but they are changing it to allow sync in cases where UI feedback needs to be immediate. It sounds familiar to what we have in case of Gutenberg :)

packages/editor/src/components/block-list/index.js

sgomes

Great PR, @youknowriad! I like the approach, and it generally looks good, apart from a few minor comments I added.

Let me know when you feel this is ready for some perf measurements and I'll take some numbers!

sgomes · 2019-01-07T15:00:26Z

packages/tests-e2e/specs/meta-attribute-block.test.js

@@ -40,6 +40,9 @@ describe( 'Block with a meta attribute', () => {
 		await insertBlock( 'Test Meta Attribute Block' );
 		await page.keyboard.type( 'Meta Value' );

+		// Wait for all async blocks to update
+		await new Promise( ( resolve ) => setTimeout( resolve, 500 ) );


I'm not sure this does what the comment says it does.

Does the guarantee that all blocks are updated come from the fact that we're waiting for the current cycle of the event loop to end and the next one to begin? If so, you can use a timeout of zero and skip waiting for 500ms. Otherwise, those 500ms offer no guarantee of completion and a different approach will be needed to ensure this always works.

I realise this is just a test, but I could see this turning into a hard-to-debug flaky test in the future :-/

In async mode, each single component is updated in its own cycle. which mean I don't know the exact number of cycles required here. But yeah, maybe there's a better way.

Can we force components to update synchronously in a test environment?

This feels very concerning to me as is.

We could it but I personally prefer not to because it creates a difference in behavior for tests we may not be aware of. For example, I did find a lot of "bugs" when I originally implemented the async mode thanks to the e2e tests which we won't catch if we use sync mode in tests.

In that case, maybe it's worth having a separate test case which could target and specifically verify some expected difference between synchronous and asynchronous modes, or run all / a subset of tests under each, or abstract the problematic interactions under a "safe" variant which handles the expected delay.

What I don't think we should start doing is arbitrarily littering some tests with randomly chosen timeouts which may or may not be sufficient time to complete the asynchronous task (fragile), and may or may not needlessly delay the total runtime of the tests.

I can rewrite the tests slightly to avoid it.

Select the block and then check it's value because selecting a block triggers the sync mode.

sgomes · 2019-01-07T15:17:47Z

packages/data/src/components/with-select/index.js

+	const nextComponent = waitingList.shift();
+	componentsMap.get( nextComponent )();
+	componentsMap.delete( nextComponent );
+	window.requestAnimationFrame( runWaitingList );


I'm not sure a rAF is what you want here; this doesn't look like it should be tied to the screen's refresh rate.

What's the goal behind this wait? If you're just looking to defer things until the next run of the event loop, I'd use a simple lodash defer or the equivalent setTimeout() with a timeout of zero.

Do you instead want to make sure it doesn't all happen in one go and only when the browser is otherwise free to process things? If so, I'd use requestIdleCallback instead, which is only executed when the browser has spare time, with a polyfill or fallback of some sort for browsers that don't support it.

I think React's async mode uses requestAnimationFrame as well. I'm not certain what's the best approach yet but the idea is that when you're typing or moving the mouse or resizing, I want to leave the priority to the browser to do the most critical things.

That sounds like a good use-case for requestIdleCallback, then. Which as far as I can tell is what React's async mode uses as well, but I may have been looking at old code.

The motivation against using requestAnimationFrame is that you'd be doing non-animation work when the browser is getting ready to create a frame. This could lead to one of two things:

If the work involved in each of these is complex, you could blow the 13ms budget and cause jank.

If the work involved in each of these is not complex, you could be wasting time by taking numberOfItemsInQueue * 13ms to process the whole queue, when you could potentially be processing it all in much less time.

requestIdleCallback instead schedules things to be done when the browser expects it will have at least 50ms (I believe), which gives you a bigger budget for complex changes and allows for running multiple chunks of work in succession without waiting for another cycle if they're small enough. This could help in keeping changes coherent, too, instead of potentially having one component update per frame.

Looks like requestIdleCallback is not supported by the browsers we need, and I'm not certain it has polyfills.

React uses requestAnimationFrame to provide some kind of polyfill.

https://github.com/facebook/react/blob/653bc582f94e955b175d42836479ee8b9eeeeeaf/packages/scheduler/src/Scheduler.js

I'd be OK using rIC where supported and just having a polyfill as simple as rIC = ( ...args ) => requestAnimationFrame( ...args ) elsewhere, but I'll leave this up to you. As long as the numbers improve, that's what matters :)

While your code is good, I still have more confidence personally in using requestAnimationFrame for this reason:

It seems that in your code the queue will be cleared by batch which mean it will have two consequences:

While executing a single batch (I guess it's 50ms duration), we can't perform incoming higher-priority work (typing)

After each batch, we'll wait another 50ms for the next batch.

Anyway I updated using rAF.

No problem, I'll test as-is (using rAF) :) Again, if the improvements are there and it works correctly, it's already a win!

I just want to clarify that higher-priority work isn't an issue in the rIC case; that's the point of checking timeRemaining() after each item, and moving onto a new batch if there's no time left. We're trusting the browser to tell us there's no time left if more important work needs to be done.

As for waiting 50ms, yes, that's true, the wait is longer than the 13ms of a rAF. However, timing is often about waiting for the right time to do something, even if it takes a little longer. That's exactly what rIC tries to do, by using timing knowledge that's internal to the browser. Besides, in the rAF case, you'll incur the 13ms penalty between every item, rather than 50ms between batches in rIC. And since you expect each component's withSelect to be fast (which I assume to mean significantly less than 13ms), that likely means it'll be slower in aggregate, since the average batch would only need to have around 4 items for the rIC case to be faster.

Thanks for the clarifications :) I'm convinced now

Sweet, thank you! :D I should be able to get you some numbers today! :)

React uses requestAnimationFrame to implement polyfill for requestIdleCallback, which would otherwise be exactly the function they want to call.

The goal is to run code as soon as possible after a frame is painted. That's achieved with:

Scheduling a callback with requestAnimationFrame. But that alone schedules the callback right before a frame is painted, the opposite of what we want to achieve. That's why this callback just posts a message to the window.

Then there is a window.onmessage handler that does the real work. If a message is posted inside requestAnimationFrame, it's guaranteed to be processed ASAP, but not before the frame is painted. It'll be the first thing the browser will do after painting! Mission accomplished.

requestAnimationFrame callback, i.e., running code before frame paint, is useful for writing to DOM effectively, without causing layout trashing.

Running code after paint is useful for effective reading from DOM, without causing layout trashing.

React Hooks introduce new APIs for these purposes. useEffect is ran before paint and is good for DOM writing, and useLayoutEffect is ran after paint and is good for DOM reading. Or is it the other way around? I'm not sure 😄

Calypso has an afterLayoutFlush utility function that follows very closely the React implementation to schedule stuff after paint. But it uses setTimeout(0) instead of postMessage, because I didn't know the hairy details a year ago. setTimeout(0) has the disadvantage that it can skip many frames before running, e.g., when scrolling.

packages/data/src/components/with-select/index.js

youknowriad · 2019-01-07T15:27:53Z

@sgomes I feel this is ready for performance measurements

packages/data/package.json

sgomes · 2019-01-08T18:08:38Z

I'm still figuring out how best to measure things here, but I'm seeing a definite improvement in terms of responsiveness. In the old code, there's a big monolithic set of changes that causes a huge 2.4 second frame, in my test case (typing in a large document with a 6x CPU throttling multiplier):

In the new code, the work doesn't take as long (1.7s), but more importantly it's split up into multiple frames, which allow for more visual feedback and thus higher responsiveness:

This evidence is mostly anecdotal, as I'd still like to find a good way of running repeated tests and getting some more accurate numbers out. Not sure how I'll do that yet, but it'll likely involve Puppeteer :)

jsnajdr · 2019-01-09T14:44:31Z

When reading the patch, this is how I understand it: when the store changes and listeners are fired, the listeners registered by AsyncModeConsumer are not called synchronously, but are deferred to a later event loop tick. Is that right?

That's almost exactly what setState in React Concurrent Mode will do after it reaches a stable version and starts to be used in Gutenberg. That's both good news (it validates this approach) and also bad news: this PR will become obsolete once React starts doing the same thing internally.

youknowriad · 2019-01-10T07:56:53Z

@jsnajdr Actually, the principles are the same but there's a small and subtle difference here that makes a big difference comparing to React Async Mode and won't be solved by it.

The main performance problem while the number of components grow in a React/Redux application is not the rerendering of all the components as most likely these components already have logic (pure and similar) to avoid rerendering them if the props/state don't change. That's exactly what we already have in Gutenberg. So if I type in a block, there's only the current block that rerenders with some chrome components but the other blocks don't.

The main issue comes from the fact that we're obliged to run the selectors (even if memoized and fast) for all the components and all the blocks to determine the next props and whether we should rerender or not. And the selectors calls happens before setState call which means the async rendering of React won't have any effect here.

sgomes · 2019-01-10T11:06:21Z

This feels like a good solution, given the constraints, and I agree with @youknowriad that this solves issues at a different layer than what React's async mode does. I completely agree with the assertion that the issue in Gutenberg is not that components update too often, but rather that too many selectors are running too often, and that can only be solved at the data layer.

It's unfortunate, though. The more I learn about Redux (and particularly the way it interacts with React), the narrower the scope of use-cases that it seems to be an optimum solution for feels.

Gutenberg has already needed a number of non-trivial architectural workarounds, beyond the expected selector performance tweaking: multiple stores; a custom implementation of the React bindings that only runs selectors when the state tree mutates (withSelect); tweaks to the data structure to avoid performance issues; and now a new layer of "state" management on top of Redux, so that some components only get their selectors run some of the time.

And this is by no means a "write and forget" thing. It will take constant vigilance to make sure all these pieces keep functioning optimally :-/

jsnajdr · 2019-01-11T10:53:13Z

@youknowriad Thanks for the clarification! I was wondering whether the performance sensitive part is running of the selectors, or the actual rerender. React will help only with the latter of course 😞

Do I understand correctly that the main goal of this change is to make typing more responsive?

Doing a little profiling (in the Calypso version) I see a lot of things is happening on every keystroke. The isEditedPostBeingScheduled selector creates moment date objects, the "Preview" button in Calypso integration even forces full content serialization on every keystroke, because of the getEditedPostAttribute( 'content' ) selector. (:wave: @Copons)

The Calypso Classic editor, the one that uses TinyMCE, isolates the content change on typing inside the TinyMCE component and synchronizes the content to Redux only later, when the user stops typing. Using debounce.

Would that be an option for typing inside a block, too? Dispatching an UPDATE_BLOCK_ATTRIBUTES action to the core/editor on every keystroke seems to be inefficient. Many things can happen in core/editor, many listeners listen for changes, but a single keystroke rarely changes anything the listeners are interested in. Can we have a separate reducer or store for every block? Or handle the typing in local state?

sgomes · 2019-01-11T11:08:00Z

@jsnajdr: FYI, I submitted a trial PR adding debouncing to input: #12592. Or rather, throttling, although as I explain there, I'm open to changing that to debouncing.

aduth · 2019-01-11T18:27:32Z

packages/editor/src/components/block-list/index.js

+						<AsyncModeProvider
+							key={ 'block-' + clientId }
+							value={
+								selectedBlockClientId !== clientId &&


What is this value representing? For clarity and legibility, it'd be deserving of being assigned a variable.

It represents when we enable/disable the async mode, I can add a clarifying comments. The idea is that selected blocks are sync, the others are async.

Well, from how I interpret it, it represents whether the rendered block occurs within the current selection, something akin to a isBlockInSelection variable (or even selector, though I don't think this can help here as written).

aduth · 2019-01-11T18:28:49Z

packages/editor/src/components/block-list/index.js

+				{ map( blockClientIds, ( clientId, blockIndex ) => {
+					return (
+						<AsyncModeProvider
+							key={ 'block-' + clientId }


I know we had it previously, but I wonder why it matters to have the key be prefixed.

packages/priority-queue/README.md

packages/priority-queue/src/index.js

aduth · 2019-01-11T18:46:20Z

packages/tests-e2e/specs/meta-attribute-block.test.js

@@ -40,6 +40,9 @@ describe( 'Block with a meta attribute', () => {
 		await insertBlock( 'Test Meta Attribute Block' );
 		await page.keyboard.type( 'Meta Value' );

+		// Wait for all async blocks to update
+		await new Promise( ( resolve ) => setTimeout( resolve, 500 ) );


Can we force components to update synchronously in a test environment?

This feels very concerning to me as is.

packages/priority-queue/src/index.js

packages/priority-queue/README.md

aduth · 2019-01-15T17:25:27Z

packages/editor/src/components/block-list/index.js

+						<AsyncModeProvider
+							key={ 'block-' + clientId }
+							value={
+								selectedBlockClientId !== clientId &&


Well, from how I interpret it, it represents whether the rendered block occurs within the current selection, something akin to a isBlockInSelection variable (or even selector, though I don't think this can help here as written).

packages/editor/src/components/block-list/index.js

packages/e2e-tests/specs/plugins/meta-attribute-block.test.js

packages/data/src/components/with-select/index.js

youknowriad · 2019-01-15T18:45:59Z

😮

aduth · 2019-12-17T21:10:32Z

packages/priority-queue/src/index.js

+		}
+	};
+
+	const flush = ( element ) => {


I don't know why it didn't occur to me previously, but: In this context, wouldn't a flush method be expected to run the callback immediately? With this implementation, it acts more like a "delete" than a "flush".

I guess the call was being done regardless but on the caller side. Having it here could simplify things.

youknowriad added the [Type] Performance Related to performance efforts label Dec 21, 2018

youknowriad self-assigned this Dec 21, 2018

youknowriad requested review from sgomes, gziolo, aduth and a team December 21, 2018 11:17

youknowriad force-pushed the try/data-module-async-mode branch from 7317fb3 to 8b9d156 Compare December 21, 2018 11:27

youknowriad force-pushed the try/data-module-async-mode branch 2 times, most recently from 0acd916 to 14b2cbe Compare January 7, 2019 13:31

youknowriad changed the title ~~WIP: Try async mode in the data module~~ Async mode in the data module Jan 7, 2019

gziolo reviewed Jan 7, 2019

View reviewed changes

packages/editor/src/components/block-list/index.js Outdated Show resolved Hide resolved

packages/editor/src/components/block-list/index.js Show resolved Hide resolved

sgomes reviewed Jan 7, 2019

View reviewed changes

gziolo reviewed Jan 8, 2019

View reviewed changes

packages/data/package.json Show resolved Hide resolved

aduth reviewed Jan 11, 2019

View reviewed changes

youknowriad added 8 commits January 15, 2019 17:16

Try async mode in the data module

aef38be

Use sync mode for the currently selected block

6094bb7

Render multi-selected blocks synchronously

2dddab3

Update components if rendering mode changes

f3b00dc

Fix async mode condition

379460e

Fix multi-selection test

dba3abc

Fix the nexted writing flow issue

ac7a087

Fix meta attributes test

8d09109

youknowriad added 8 commits January 15, 2019 17:16

remove useless code

f03f906

Extract the priority queue module

9caaf53

Declaring the php script for the priority-queue module

4dc44dc

mark the async mode as experimental

81792f5

Use requestAnimationFrame

f48dffa

Optimize using requestIdleCallback

63e80b9

Avoid timeouts in the meta block tests

abdbdaf

Tweaks the README

6b4a1b8

youknowriad force-pushed the try/data-module-async-mode branch from 57dc295 to 6b4a1b8 Compare January 15, 2019 17:16

aduth reviewed Jan 15, 2019

View reviewed changes

youknowriad and others added 2 commits January 15, 2019 17:45

Tweaks per review

243e51d

Block List: Extract selection condition to variable

1c8b0d8

aduth approved these changes Jan 15, 2019

View reviewed changes

youknowriad merged commit 3c5fd22 into master Jan 15, 2019

youknowriad deleted the try/data-module-async-mode branch January 15, 2019 18:45

youknowriad added this to the 4.9 (Gutenberg) milestone Jan 15, 2019

youknowriad added a commit that referenced this pull request Mar 6, 2019

Async mode in the data module (#13056)

6adc907

youknowriad added a commit that referenced this pull request Mar 6, 2019

Async mode in the data module (#13056)

7f1966d

ellatrix mentioned this pull request Apr 18, 2019

Thoughts on RichText & Performance #15033

Closed

youknowriad mentioned this pull request May 20, 2019

Data: Implement useSelect hooks corollary to withSelect #15473

Closed

chrisvanpatten mentioned this pull request Sep 29, 2019

Allow forcing synchronous block rendering #17638

Closed

aduth reviewed Dec 17, 2019

View reviewed changes

This was referenced Dec 17, 2019

Data: Invalidate store subscription on change in isAsync #19205

Closed

Different timing of updates between withSelect and useSelect #19199

Closed

Priority Queue: Invoke callback when flushing queue #19282

Merged

Async mode in the data module #13056

Async mode in the data module #13056

Conversation

youknowriad commented Dec 21, 2018 • edited Loading

gziolo commented Dec 22, 2018

sgomes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgomes Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnajdr Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

youknowriad commented Jan 7, 2019

sgomes commented Jan 8, 2019

jsnajdr commented Jan 9, 2019

youknowriad commented Jan 10, 2019

sgomes commented Jan 10, 2019 • edited Loading

jsnajdr commented Jan 11, 2019

sgomes commented Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jan 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Dec 21, 2018 •

edited

Loading

sgomes Jan 8, 2019 •

edited

Loading

jsnajdr Jan 9, 2019 •

edited

Loading

sgomes commented Jan 10, 2019 •

edited

Loading

sgomes commented Jan 11, 2019 •

edited

Loading