Effects list refactor: Move resetChildLanes into complete work #19440

bvaughn · 2020-07-23T20:38:49Z

Builds on top of #19374

Move resetChildLanes out of work loop and into complete work (named bubbleProperties)
Move tag-type checks in bubbleProperties into their corresponding switch/case statements.
Change new bubbleProperties method to bubble up to the parent as each Fiber completes, rather than iterating over its children. (This will enable us to remove a potentially expensive while loop.)

codesandbox-ci · 2020-07-23T20:41:06Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c7c0203:

Sandbox	Source
React	Configuration

bvaughn · 2020-07-23T20:41:20Z

packages/react-reconciler/src/ReactFiberCompleteWork.new.js

@@ -649,6 +671,124 @@ function cutOffTailIfNeeded(
 }
 }

+function bubbleProperties(completedWork: Fiber): void {


This isn't the greatest name, but neither was resetChildLanes. Open to suggestions.

sizebot · 2020-07-23T20:49:09Z

Details of bundled changes.

Comparing: d38ec17...c7c0203

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	0.0%	0.0%	905.31 KB	905.31 KB	206.17 KB	206.17 KB	NODE_DEV
ReactDOMForked-prod.js	🔺+0.2%	🔺+0.2%	376.78 KB	377.54 KB	69.97 KB	70.1 KB	FB_WWW_PROD
react-dom-server.node.development.js	0.0%	0.0%	138.57 KB	138.57 KB	36.59 KB	36.59 KB	NODE_DEV
ReactDOMForked-profiling.js	-0.1%	-0.0%	390.22 KB	389.96 KB	72.45 KB	72.43 KB	FB_WWW_PROFILING
react-dom-server.node.production.min.js	0.0%	0.0%	20.66 KB	20.66 KB	7.65 KB	7.65 KB	NODE_PROD
react-dom-test-utils.production.min.js	0.0%	0.0%	12.84 KB	12.84 KB	5.04 KB	5.05 KB	UMD_PROD
react-dom-test-utils.development.js	0.0%	0.0%	61.09 KB	61.09 KB	17.75 KB	17.75 KB	NODE_DEV
react-dom-unstable-fizz.node.development.js	0.0%	-0.1%	5.52 KB	5.52 KB	1.84 KB	1.84 KB	NODE_DEV
react-dom-test-utils.production.min.js	0.0%	0.0%	12.68 KB	12.68 KB	4.97 KB	4.97 KB	NODE_PROD
react-dom-unstable-fizz.node.production.min.js	0.0%	🔺+0.5%	1.17 KB	1.17 KB	665 B	668 B	NODE_PROD
react-dom.development.js	0.0%	0.0%	951.32 KB	951.32 KB	208.77 KB	208.77 KB	UMD_DEV
react-dom-unstable-fizz.browser.production.min.js	0.0%	🔺+0.3%	1.01 KB	1.01 KB	615 B	617 B	NODE_PROD
react-dom.profiling.min.js	0.0%	0.0%	127.41 KB	127.41 KB	41.77 KB	41.77 KB	UMD_PROFILING
ReactDOMForked-dev.js	-0.0%	-0.0%	960.3 KB	960.11 KB	214.26 KB	214.21 KB	FB_WWW_DEV
react-dom.profiling.min.js	0.0%	0.0%	127.76 KB	127.76 KB	41.05 KB	41.05 KB	NODE_PROFILING
react-dom-server.browser.production.min.js	0.0%	0.0%	20.34 KB	20.34 KB	7.55 KB	7.55 KB	UMD_PROD
ReactDOM-dev.js	0.0%	0.0%	959.68 KB	959.68 KB	214.8 KB	214.8 KB	FB_WWW_DEV
react-dom-server.browser.development.js	0.0%	0.0%	137.3 KB	137.3 KB	36.34 KB	36.34 KB	NODE_DEV
ReactDOM-profiling.js	0.0%	0.0%	389.44 KB	389.44 KB	72.2 KB	72.2 KB	FB_WWW_PROFILING
react-dom-test-utils.development.js	0.0%	0.0%	66.1 KB	66.1 KB	18.24 KB	18.24 KB	UMD_DEV

ReactDOM: size: 0.0%, gzip: 0.0%

Size changes (experimental)

Generated by 🚫 dangerJS against c7c0203

sizebot · 2020-07-23T20:50:15Z

Details of bundled changes.

Comparing: d38ec17...c7c0203

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	0.0%	0.0%	869.86 KB	869.86 KB	199.68 KB	199.68 KB	NODE_DEV
ReactDOMForked-prod.js	🔺+0.2%	🔺+0.2%	387.95 KB	388.78 KB	71.73 KB	71.85 KB	FB_WWW_PROD
react-dom-server.node.development.js	0.0%	0.0%	137.06 KB	137.06 KB	36.37 KB	36.37 KB	NODE_DEV
ReactDOMForked-profiling.js	-0.0%	-0.1%	401.44 KB	401.24 KB	74.21 KB	74.17 KB	FB_WWW_PROFILING
react-dom-server.browser.development.js	0.0%	0.0%	143.14 KB	143.14 KB	36.57 KB	36.57 KB	UMD_DEV
react-dom-test-utils.production.min.js	0.0%	0.0%	12.83 KB	12.83 KB	5.04 KB	5.04 KB	UMD_PROD
ReactDOMTesting-dev.js	0.0%	-0.0%	941.49 KB	941.49 KB	211.57 KB	211.57 KB	FB_WWW_DEV
react-dom-test-utils.development.js	0.0%	0.0%	61.08 KB	61.08 KB	17.74 KB	17.74 KB	NODE_DEV
react-dom.development.js	0.0%	0.0%	914.12 KB	914.12 KB	202.19 KB	202.19 KB	UMD_DEV
ReactDOMForked-dev.js	-0.0%	-0.0%	985.82 KB	985.63 KB	219.04 KB	219 KB	FB_WWW_DEV
react-dom.profiling.min.js	0.0%	0.0%	121.83 KB	121.83 KB	39.28 KB	39.28 KB	NODE_PROFILING
ReactDOM-dev.js	0.0%	0.0%	985.2 KB	985.2 KB	219.53 KB	219.53 KB	FB_WWW_DEV
react-dom-server.browser.production.min.js	0.0%	0.0%	19.78 KB	19.78 KB	7.42 KB	7.42 KB	NODE_PROD
ReactDOMServer-dev.js	0.0%	0.0%	145.62 KB	145.62 KB	37.31 KB	37.31 KB	FB_WWW_DEV
ReactDOMServer-prod.js	0.0%	0.0%	47.3 KB	47.3 KB	11.03 KB	11.04 KB	FB_WWW_PROD

ReactDOM: size: 0.0%, gzip: 0.0%

Size changes (stable)

Generated by 🚫 dangerJS against c7c0203

bvaughn · 2020-07-24T14:56:29Z

Based on @acdlite's comment on PR #19381...

I'm trying to remember the reason why resetChildLanes exists. An alternative would be to reset the childLanes (and subtreeTag, and similar fields) to 0 in the begin phase, and accumulate the child values in each child fiber's complete phase. Instead of doing another traversal.

...it seems there may have been an additional refactoring goal here to bubble properties to a completed Fiber's parent (rather than the parent reading them from its children) so we could remove the child loop that currently exists. I think this is doable, but I think it would also require us to reset treeBaseDuration on a Fiber during the begin phase (when we clone) in order for the children to be able to bubble their times up correctly. Maybe that would work and maybe it would cause a problem. I'm actually not sure, but I can take a shot at it and see.

We also still loop over children to bubble actualDurations in the work loop, in the event of an error, but I assume that could stay as-is.

react/packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Lines 1795 to 1803 in 909b612

 // Include the time spent working on failed children before continuing. 

 let actualDuration = completedWork.actualDuration; 

 let child = completedWork.child; 

 while (child !== null) { 

 actualDuration += child.actualDuration; 

 child = child.sibling; 

 } 

 completedWork.actualDuration = actualDuration; 

 }

bvaughn · 2020-07-27T15:27:10Z

Regarding the treeBaseDuration issue mentioned above, I think one plausible approach might be to reset treeBaseDuration to 0 during clone, but then copy it back in the event of a bailout. However I'm running into another more pressing snag- properly bubbling childLanes from the child to the parent seems trickier than the other way around. (More specifically knowing when to reset the value is more difficult.) So far what I've tried results in either an infinite loop or skipped work.

acdlite · 2020-07-27T16:32:48Z

Regarding the treeBaseDuration issue mentioned above, I think one plausible approach might be to reset treeBaseDuration to 0 during clone, but then copy it back in the event of a bailout.

That makes sense to me.

However I'm running into another more pressing snag- properly bubbling childLanes from the child to the parent seems trickier than the other way around. (More specifically knowing when to reset the value is more difficult.)

My guess is that the issue you ran into is when a fiber bails out and you reuse the current child (i.e. returning null from beginWork instead of continuing on the child). The children won't be rendered, so they won't have a complete phase, so the values from the children won't be accumulated. So in that case, you still need to call something like resetChildLanes/bubbleProperties.

acdlite · 2020-07-27T16:53:13Z

So in that case, you still need to call something like resetChildLanes/bubbleProperties.

Actually, I forgot that in the case of a complete bailout (workInProgress.child === current.child) the bubbled values are the same as current. So you can read off the current fiber, no need to traverse children.

Basically, same thing as copying over treeBaseDuration, as you mentioned above.

bvaughn · 2020-07-27T16:54:21Z

Right. This is what I was thinking too, and what I tried, but it didn't work. Probably missed something though. I'll try again after though, I've made the static change we're discussing on the other PR.

This enabled us to remove a few hot path tag-type checks, but did not otherwise change any functionality.

Note this commit fails 5 tests. I am pushing it purely for sharing purposes.

bvaughn · 2020-09-14T14:22:34Z

Superseded by #19801

bvaughn requested review from sebmarkbage, gaearon and acdlite July 23, 2020 20:38

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 23, 2020

bvaughn changed the title ~~Effects list refactor reset child lanes~~ Effects list refactor: Move resetChildLanes into complete work Jul 23, 2020

bvaughn commented Jul 23, 2020

View reviewed changes

bvaughn force-pushed the effects_list_refactor_reset_child_lanes branch from 002e2b4 to d1c9eab Compare July 23, 2020 20:43

bvaughn force-pushed the effects_list_refactor_reset_child_lanes branch from d1c9eab to 2eb2ccd Compare July 24, 2020 14:28

bvaughn force-pushed the effects_list_refactor_reset_child_lanes branch 5 times, most recently from 9faa091 to ac157d0 Compare July 29, 2020 18:00

bvaughn marked this pull request as draft July 30, 2020 13:11

bvaughn force-pushed the effects_list_refactor_reset_child_lanes branch 2 times, most recently from 3490152 to 5864d15 Compare July 30, 2020 13:19

bvaughn force-pushed the effects_list_refactor_reset_child_lanes branch 2 times, most recently from 8f0a977 to 7cbd6a1 Compare September 8, 2020 14:30

Moved resetChildLanes into complete work

a789939

This enabled us to remove a few hot path tag-type checks, but did not otherwise change any functionality.

bvaughn force-pushed the effects_list_refactor_reset_child_lanes branch from 7cbd6a1 to a789939 Compare September 8, 2020 14:38

Bubble profiler durations to parent (rather than walking children)

c7c0203

Note this commit fails 5 tests. I am pushing it purely for sharing purposes.

bvaughn mentioned this pull request Sep 9, 2020

[draft] Move resetChildLanes into complete work #19801

Closed

7 tasks

bvaughn closed this Sep 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Effects list refactor: Move resetChildLanes into complete work #19440

Effects list refactor: Move resetChildLanes into complete work #19440

bvaughn commented Jul 23, 2020 •

edited

Loading

codesandbox-ci bot commented Jul 23, 2020 •

edited

Loading

bvaughn Jul 23, 2020

sizebot commented Jul 23, 2020 •

edited

Loading

react-dom

sizebot commented Jul 23, 2020 •

edited

Loading

react-dom

bvaughn commented Jul 24, 2020 •

edited

Loading

bvaughn commented Jul 27, 2020 •

edited

Loading

acdlite commented Jul 27, 2020

acdlite commented Jul 27, 2020 •

edited

Loading

bvaughn commented Jul 27, 2020

bvaughn commented Sep 14, 2020

Effects list refactor: Move resetChildLanes into complete work #19440

Effects list refactor: Move resetChildLanes into complete work #19440

Conversation

bvaughn commented Jul 23, 2020 • edited Loading

codesandbox-ci bot commented Jul 23, 2020 • edited Loading

bvaughn Jul 23, 2020

Choose a reason for hiding this comment

sizebot commented Jul 23, 2020 • edited Loading

react-dom

Size changes (experimental)

sizebot commented Jul 23, 2020 • edited Loading

react-dom

Size changes (stable)

bvaughn commented Jul 24, 2020 • edited Loading

bvaughn commented Jul 27, 2020 • edited Loading

acdlite commented Jul 27, 2020

acdlite commented Jul 27, 2020 • edited Loading

bvaughn commented Jul 27, 2020

bvaughn commented Sep 14, 2020

bvaughn commented Jul 23, 2020 •

edited

Loading

codesandbox-ci bot commented Jul 23, 2020 •

edited

Loading

sizebot commented Jul 23, 2020 •

edited

Loading

sizebot commented Jul 23, 2020 •

edited

Loading

bvaughn commented Jul 24, 2020 •

edited

Loading

bvaughn commented Jul 27, 2020 •

edited

Loading

acdlite commented Jul 27, 2020 •

edited

Loading