Decouple update queue from Fiber type #12600

acdlite · 2018-04-10T21:01:12Z

The update queue is in need of a refactor. Recent bugfixes (#12528) have exposed some flaws in how it's modeled. Upcoming features like Suspense and [redacted] also rely on the update queue in ways that weren't anticipated in the original design.

Major changes:

Instead of boolean flags for isReplace and isForceUpdate, updates have a tag field (like Fiber). This lowers the cost for adding new types of updates.
Render phase updates are special cased. Updates scheduled during the render phase are dropped if the work-in-progress does not commit. This is used for getDerivedStateFrom{Props,Catch}.
callbackList has been replaced with a generic effect list. Aside from callbacks, this is also used for componentDidCatch.

TODO:

Class components
Host roots
Fix Flow
~~Update React DevTools dependency on hasForceUpdate (in a separate repo)~~ I reverted this change

acdlite · 2018-04-10T21:03:21Z

...ages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap

@@ -248,7 +248,7 @@ exports[`ReactDebugFiberPerf recovers from caught errors 1`] = `
 ⛔ (Committing Changes) Warning: Lifecycle hook scheduled a cascading update
  ⚛ (Committing Snapshot Effects: 0 Total)
  ⚛ (Committing Host Effects: 2 Total)
-  ⚛ (Calling Lifecycle Methods: 0 Total)
+  ⚛ (Calling Lifecycle Methods: 1 Total)


This is because we weren't counting componentDidCatch as a lifecycle previously

pull-bot · 2018-04-10T21:09:52Z

ReactDOM: size: 🔺+0.4%, gzip: -0.2%

Details of bundled changes.

Comparing: 999b656...e4ce363

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.3%	+0.6%	609.63 KB	611.61 KB	140.53 KB	141.35 KB	UMD_DEV
react-dom.production.min.js	🔺+0.4%	-0.2%	100.2 KB	100.56 KB	31.97 KB	31.9 KB	UMD_PROD
react-dom.development.js	+0.3%	+0.6%	594 KB	595.99 KB	136.37 KB	137.22 KB	NODE_DEV
react-dom.production.min.js	🔺+0.3%	0.0%	98.65 KB	98.99 KB	31.1 KB	31.11 KB	NODE_PROD
react-dom-test-utils.development.js	-0.0%	0.0%	40.75 KB	40.75 KB	11.69 KB	11.69 KB	UMD_DEV
react-dom-test-utils.development.js	-0.0%	-0.0%	35.61 KB	35.61 KB	10.27 KB	10.27 KB	NODE_DEV
ReactDOM-dev.js	+0.3%	+0.6%	618.41 KB	620.4 KB	139.14 KB	139.94 KB	FB_WWW_DEV
ReactDOM-prod.js	-0.1%	-0.4%	284.94 KB	284.78 KB	52.3 KB	52.09 KB	FB_WWW_PROD

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.5%	+0.9%	412.73 KB	414.71 KB	89.59 KB	90.41 KB	UMD_DEV
react-art.production.min.js	🔺+0.4%	-0.3%	90.48 KB	90.85 KB	27.62 KB	27.54 KB	UMD_PROD
react-art.development.js	+0.6%	+1.1%	338.56 KB	340.55 KB	70.81 KB	71.62 KB	NODE_DEV
react-art.production.min.js	🔺+0.6%	-0.5%	55.04 KB	55.37 KB	16.87 KB	16.79 KB	NODE_PROD
ReactART-dev.js	+0.6%	+1.1%	346.81 KB	348.8 KB	70.42 KB	71.18 KB	FB_WWW_DEV
ReactART-prod.js	-0.1%	-0.7%	167.61 KB	167.48 KB	27.75 KB	27.57 KB	FB_WWW_PROD

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+0.6%	+1.1%	345.69 KB	347.67 KB	72.47 KB	73.28 KB	UMD_DEV
react-test-renderer.production.min.js	🔺+0.6%	-0.2%	54.77 KB	55.13 KB	16.71 KB	16.68 KB	UMD_PROD
react-test-renderer.development.js	+0.6%	+1.2%	336.51 KB	338.5 KB	69.66 KB	70.46 KB	NODE_DEV
react-test-renderer.production.min.js	🔺+0.6%	-0.1%	54.02 KB	54.36 KB	16.35 KB	16.33 KB	NODE_PROD
ReactTestRenderer-dev.js	+0.6%	+1.1%	344.98 KB	346.97 KB	69.31 KB	70.09 KB	FB_WWW_DEV

react-reconciler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-reconciler.development.js	+0.6%	+1.2%	316.9 KB	318.89 KB	65.12 KB	65.92 KB	NODE_DEV
react-reconciler.production.min.js	🔺+0.7%	-0.5%	46.91 KB	47.24 KB	14.3 KB	14.23 KB	NODE_PROD
react-reconciler-persistent.development.js	+0.6%	+1.2%	316.23 KB	318.22 KB	64.88 KB	65.69 KB	NODE_DEV
react-reconciler-persistent.production.min.js	🔺+0.7%	🔺+0.2%	45.83 KB	46.16 KB	14.08 KB	14.11 KB	NODE_PROD
react-reconciler-reflection.development.js	-0.0%	0.0%	11.06 KB	11.06 KB	3.42 KB	3.42 KB	NODE_DEV

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.4%	+0.8%	456.63 KB	458.64 KB	97.38 KB	98.15 KB	RN_FB_DEV
ReactNativeRenderer-prod.js	-0.3%	-0.7%	218.26 KB	217.69 KB	36.66 KB	36.39 KB	RN_FB_PROD
ReactNativeRenderer-dev.js	+0.4%	+0.8%	456.38 KB	458.39 KB	97.32 KB	98.08 KB	RN_OSS_DEV
ReactNativeRenderer-prod.js	🔺+0.9%	-0.1%	215.01 KB	216.91 KB	36.29 KB	36.26 KB	RN_OSS_PROD
ReactFabric-dev.js	+0.5%	+0.9%	439.04 KB	441.05 KB	92.94 KB	93.75 KB	RN_FB_DEV
ReactFabric-prod.js	🔺+1.0%	🔺+0.3%	200.59 KB	202.5 KB	33.55 KB	33.65 KB	RN_FB_PROD
ReactFabric-dev.js	+0.5%	+0.9%	439.08 KB	441.09 KB	92.96 KB	93.76 KB	RN_OSS_DEV
ReactFabric-prod.js	🔺+1.0%	🔺+0.3%	200.63 KB	202.54 KB	33.57 KB	33.67 KB	RN_OSS_PROD

Generated by 🚫 dangerJS

gaearon · 2018-04-10T21:12:48Z

Need to add “update DevTools” to todos. I think it might rely on isForceUpdate somewhere.

bvaughn · 2018-04-10T21:17:55Z

Need to add “update DevTools” to todos. I think it might rely on isForceUpdate somewhere.

I don't think so? I don't see any references to isForced.

gaearon · 2018-04-10T21:45:25Z

Oops, it was hasForceUpdate.

https://github.com/facebook/react-devtools/blob/f0cde12e3531aae203f8a685f5e74ead58019b0a/backend/attachRendererFiber.js#L57

acdlite · 2018-04-10T22:07:19Z

Note: the bundle size changes are mostly because I haven't deleted the old update queue yet :D

acdlite · 2018-04-11T02:27:29Z

@sebmarkbage Ready for review

acdlite · 2018-04-11T02:31:13Z

...ages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap

-  ⚛ (Committing Host Effects: 1 Total)
-  ⚛ (Calling Lifecycle Methods: 1 Total)
+  ⚛ (Committing Host Effects: 0 Total)
+  ⚛ (Calling Lifecycle Methods: 0 Total)


I don't know why this wasn't already 0. There are no host effects or lifecycles in the second flush. New snapshot seems correct.

acdlite · 2018-04-11T02:31:17Z

...ages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap

@@ -274,7 +274,7 @@ exports[`ReactDebugFiberPerf recovers from fatal errors 1`] = `
 ⚛ (Committing Changes)
  ⚛ (Committing Snapshot Effects: 0 Total)
  ⚛ (Committing Host Effects: 1 Total)
-  ⚛ (Calling Lifecycle Methods: 0 Total)
+  ⚛ (Calling Lifecycle Methods: 1 Total)


acdlite · 2018-04-11T02:50:08Z

Needless to say, this is a risky change. I'll do the next www sync and fix any bugs that may come up.

NE-SmallTown · 2018-04-13T08:23:32Z

Upcoming features like Suspense and [redacted] also rely on the update queue in ways that weren't anticipated in the original design.

What does [redacted] mean?

sebmarkbage · 2018-04-17T21:27:20Z

packages/react-reconciler/src/ReactFiberClassComponent.js

@@ -749,10 +684,11 @@ export default function(
  ): boolean {
    const ctor = workInProgress.type;
    const instance = workInProgress.stateNode;
-    resetInputPointers(workInProgress, instance);


What is this change? You didn't replicate the other assignment so something is new.

The other assignment is here: https://github.com/facebook/react/pull/12600/files/a99b4cae0283fd811c80b9a5b18eb59c5401dac6#diff-b69c8f2165f95ef2355ac661c12fa407R727

sebmarkbage · 2018-04-17T21:28:07Z

packages/react-reconciler/src/ReactFiberClassComponent.js

-      Object.assign(resultState, partialState);
-    }
+
+function enqueueDerivedStateFromProps(


DRY for DRYness sake? Makes it harder to read this inline IMO. It also encourages reading the getDerivedStateFromProps property in multiple places if they're also needed at the callsite. Only abstract if it is chunk of work. (On the flip side, this PR successfully unabstract memoizeProps and memoizeState in places.)

sebmarkbage · 2018-04-17T21:37:30Z

packages/react-reconciler/src/ReactFiberCompleteWork.js

-        if (updateQueue !== null && updateQueue.capturedValues !== null) {
-          workInProgress.effectTag &= ~DidCapture;
-          if (typeof instance.componentDidCatch === 'function') {
-            workInProgress.effectTag |= ErrLog;


There was a reason we had to apply these effects in the complete phase. Why are we able to move them to the begin phase now?

I don't think there was a reason. It happened to be a convenient place to do it. Now we do it while processing the update queue.

sebmarkbage · 2018-04-17T21:49:45Z

packages/shared/ReactTypeOfSideEffect.js

@@ -19,14 +19,14 @@ export const Update = /*                */ 0b000000000100;
 export const PlacementAndUpdate = /*    */ 0b000000000110;
 export const Deletion = /*              */ 0b000000001000;
 export const ContentReset = /*          */ 0b000000010000;
-export const Callback = /*              */ 0b000000100000;
+export const UpdateQueue = /*           */ 0b000000100000;


What are theses? They're not described in terms of side-effects. Either this needs to change, or the whole thing renamed.

sebmarkbage · 2018-04-18T00:09:19Z

packages/react-reconciler/src/ReactUpdateQueue.js

+const ClassUpdateQueue = 0;
+const RootUpdateQueue = 1;
+
+type UpdateShared<Payload, U> = {


I really had the term "Shared". This has been sneaking into RN too.

It doesn't describe anything about the concept other than DRYness which indicates that maybe you should repeat yourself. If it is real concept, name it.

Stop denying me muh mixins!

sebmarkbage · 2018-04-18T00:12:16Z

packages/react-reconciler/src/ReactUpdateQueue.js

+type RootUpdateQueueType = UpdateQueueShared<RootUpdate, ReactNodeList>;
+
+type UpdateQueueOwner<Queue, State> = {
+  alternate: UpdateQueueOwner<Queue> | null,


This is clearly not going to be how we describe "redacted" so seems like a too early abstraction. Even just assuming that it is a single argument is a big assumption. Just use Fiber until you have something else.

sebmarkbage · 2018-04-18T00:20:08Z

packages/react-reconciler/src/ReactUpdateQueue.js

+      }
+      // Clear the list of render phase updates.
+      finishedQueue.firstRenderPhaseUpdate = finishedQueue.lastRenderPhaseUpdate = null;
+    }


You are forking into a first class concept with typeOfUpdateQueue then you're picking separate code paths again in commitEffect. The only thing gain by doing that instead of specializing each function is that you can reuse the code above here. Just break this out into a reusable function and instead specialize each code path without making typeOfUpdateQueue a first class thing.

acdlite · 2018-04-18T01:19:51Z

TODO after chatting to @sebmarkbage:

Use first-class functions instead of typeOfUpdateQueue
Separate effect list into "own" effects and children effects
Rename UpdateQueue side effect type to something that makes more sense
Remove ForceUpdate effect and go back to queue.hasForceUpdate instead

The update queue is in need of a refactor. Recent bugfixes (facebook#12528) have exposed some flaws in how it's modeled. Upcoming features like Suspense and [redacted] also rely on the update queue in ways that weren't anticipated in the original design. Major changes: - Instead of boolean flags for `isReplace` and `isForceUpdate`, updates have a `tag` field (like Fiber). This lowers the cost for adding new types of updates. - Render phase updates are special cased. Updates scheduled during the render phase are dropped if the work-in-progress does not commit. This is used for `getDerivedStateFrom{Props,Catch}`. - `callbackList` has been replaced with a generic effect list. Aside from callbacks, this is also used for `componentDidCatch`.

I tried to avoid this at first, since we avoid it everywhere else in the Fiber codebase, but since updates are not in a hot path, the trade off with file size seems worth it.

acdlite requested review from bvaughn and sebmarkbage April 10, 2018 21:01

facebook-github-bot added the CLA Signed label Apr 10, 2018

acdlite force-pushed the refactor-updatequeue branch from 6fdc713 to 9ae2db3 Compare April 10, 2018 21:02

acdlite commented Apr 10, 2018

View reviewed changes

acdlite force-pushed the refactor-updatequeue branch 3 times, most recently from e325697 to a99b4ca Compare April 11, 2018 02:23

acdlite changed the title ~~[WIP] Decouple update queue from Fiber type~~ Decouple update queue from Fiber type Apr 11, 2018

acdlite commented Apr 11, 2018

View reviewed changes

gaearon mentioned this pull request Apr 11, 2018

[Danger] Minor fixes #12606

Merged

acdlite mentioned this pull request Apr 14, 2018

Suspense #12279

Merged

5 tasks

sebmarkbage reviewed Apr 18, 2018

View reviewed changes

acdlite force-pushed the refactor-updatequeue branch 4 times, most recently from 995a437 to 73d4d48 Compare April 20, 2018 22:57

acdlite force-pushed the refactor-updatequeue branch from 73d4d48 to 8830fd2 Compare April 20, 2018 23:04

Remove first class UpdateQueue types and use closures instead

de45514

I tried to avoid this at first, since we avoid it everywhere else in the Fiber codebase, but since updates are not in a hot path, the trade off with file size seems worth it.

This was referenced Jun 9, 2018

Update yarn.lock kornicameister/korni#384

Closed

Update yarn.lock kornicameister/korni#385

Closed

Update yarn.lock kornicameister/korni#387

Closed

Update yarn.lock dropseed/jump#109

Closed

Update yarn.lock kornicameister/korni#390

Closed

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple update queue from Fiber type #12600

Decouple update queue from Fiber type #12600

acdlite commented Apr 10, 2018 •

edited

acdlite Apr 10, 2018

pull-bot commented Apr 10, 2018 •

edited

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

gaearon commented Apr 10, 2018

bvaughn commented Apr 10, 2018 •

edited

gaearon commented Apr 10, 2018

acdlite commented Apr 10, 2018

acdlite commented Apr 11, 2018

acdlite Apr 11, 2018

acdlite Apr 11, 2018

acdlite commented Apr 11, 2018

NE-SmallTown commented Apr 13, 2018

sebmarkbage Apr 17, 2018

acdlite Apr 20, 2018

sebmarkbage Apr 17, 2018

sebmarkbage Apr 17, 2018

acdlite Apr 20, 2018 •

edited

sebmarkbage Apr 17, 2018

sebmarkbage Apr 18, 2018

acdlite Apr 18, 2018

sebmarkbage Apr 18, 2018

sebmarkbage Apr 18, 2018

acdlite commented Apr 18, 2018 •

edited

Decouple update queue from Fiber type #12600

Decouple update queue from Fiber type #12600

Conversation

acdlite commented Apr 10, 2018 • edited

Choose a reason for hiding this comment

pull-bot commented Apr 10, 2018 • edited

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

gaearon commented Apr 10, 2018

bvaughn commented Apr 10, 2018 • edited

gaearon commented Apr 10, 2018

acdlite commented Apr 10, 2018

acdlite commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Apr 11, 2018

NE-SmallTown commented Apr 13, 2018

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

acdlite Apr 20, 2018 • edited

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

acdlite commented Apr 18, 2018 • edited

acdlite commented Apr 10, 2018 •

edited

pull-bot commented Apr 10, 2018 •

edited

bvaughn commented Apr 10, 2018 •

edited

acdlite Apr 20, 2018 •

edited

acdlite commented Apr 18, 2018 •

edited