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.

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

Jump to bottom

[draft] Move resetChildLanes into complete work #19801

Closed
wants to merge 4 commits into from
Closed

[draft] Move resetChildLanes into complete work #19801

wants to merge 4 commits into from

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Sep 9, 2020 •
edited
Loading

Alternative to #19440.

I'm breaking this PR down into small chunks for my own benefit. (Obviously these commits will be squashed prior to merging.)

  • Move resetChildLanes out of work loop and into complete work (named bubbleProperties)
  • Move tag-type checks in bubbleProperties into their corresponding switch/case statements.
  • Temporarily split bubbleProperties up into separate functions for lanes, flags, and Profiler durations.
    • Refactor subtreeLanes to bubble from workInProgress to parent (rather than from children to workInProgress).
    • Refactor childLanes to bubble from workInProgress to parent (rather than from children to workInProgress). (I've committed a partial refactor for this, but it still has some failing tests.)
    • Refactor Profiler durations to bubble from workInProgress to parent (rather than from children to workInProgress).
  • Merge separate bubble functions back into bubbleProperties method.

Brian Vaughn added 3 commits September 8, 2020 10:38
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.
Temporarily split lanes, flags, and profiler durations into separate …
0247a8a 
…bubble methods so I can refactor each in isolation
Refactored flag bubbling to be from child to parent
2b767a4
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 9, 2020
@sizebot
Copy link

sizebot commented Sep 9, 2020 •
edited
Loading

Details of bundled changes.

Comparing: d38ec17...7f575fa

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.1% 387.95 KB 388.65 KB 71.73 KB 71.82 KB FB_WWW_PROD
ReactDOMForked-profiling.js +0.2% 0.0% 401.44 KB 402.13 KB 74.21 KB 74.22 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.development.js 0.0% 0.0% 914.12 KB 914.12 KB 202.19 KB 202.19 KB UMD_DEV
ReactDOMForked-dev.js +0.2% +0.3% 985.82 KB 988.09 KB 219.04 KB 219.67 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
ReactDOM-prod.js 0.0% 0.0% 387.45 KB 387.45 KB 71.46 KB 71.47 KB FB_WWW_PROD
ReactDOM-profiling.js 0.0% 0.0% 400.65 KB 400.65 KB 73.9 KB 73.9 KB FB_WWW_PROFILING
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-prod.js 0.0% 0.0% 47.3 KB 47.3 KB 11.03 KB 11.04 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% -0.0% 66.08 KB 66.08 KB 18.23 KB 18.23 KB UMD_DEV

ReactDOM: size: 0.0%, gzip: 0.0%

Size changes (stable)

Generated by 🚫 dangerJS against 7f575fa

@sizebot
Copy link

sizebot commented Sep 9, 2020 •
edited
Loading

Details of bundled changes.

Comparing: d38ec17...7f575fa

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.1% 376.78 KB 377.42 KB 69.97 KB 70.07 KB FB_WWW_PROD
ReactDOMForked-profiling.js +0.2% +0.1% 390.22 KB 390.85 KB 72.45 KB 72.5 KB FB_WWW_PROFILING
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
ReactDOMTesting-prod.js 0.0% 0.0% 375.01 KB 375.01 KB 70.76 KB 70.76 KB FB_WWW_PROD
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.browser.development.js 0.0% -0.1% 5.25 KB 5.25 KB 1.78 KB 1.78 KB UMD_DEV
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 4.78 KB 4.78 KB 1.68 KB 1.68 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.01 KB 1.01 KB 615 B 616 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.2% +0.3% 960.3 KB 962.57 KB 214.26 KB 214.88 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
ReactDOM-prod.js 0.0% 0.0% 376.28 KB 376.28 KB 69.76 KB 69.76 KB FB_WWW_PROD
ReactDOM-profiling.js 0.0% 0.0% 389.44 KB 389.44 KB 72.2 KB 72.2 KB FB_WWW_PROFILING

ReactDOM: size: 0.0%, gzip: 0.0%

Size changes (experimental)

Generated by 🚫 dangerJS against 7f575fa

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 9, 2020 •
edited
Loading

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 7f575fa:

Sandbox Source
React Configuration

Attempted refactoring of childLanes to be from workInProgress to parent
7f575fa 
 (rather than from children to workInProgress). This attempt isn't quite right though. Has some test failures that requires additional investigation.
@bvaughn bvaughn force-pushed the effects_list_refactor_reset_child_lanes-alternative branch from 0bafaf7 to 7f575fa Compare September 14, 2020 14:21
This was referenced Sep 14, 2020
Closed
Merged
@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 15, 2020

After some thought and discussion, we've decided this effort isn't worth the time and complexity it requires right now.

Updating flags to bubble from the currently-completed node to the parent was relatively straight forward (2b767a4) but both the Profiler durations (c7c0203) and the lanes/childLanes (7f575fa) changes revealed several tricky edge cases, particularly regarding suspense and suspense list APIs.

For now, I'm going to move forward with the initial small change this branch started with (moving resetChildLanes into complete work) and we'll leave the rest for a future refactor if we decide to pick it back up.

Closing in favor of PR #19836

@bvaughn bvaughn closed this Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bvaughn @sizebot @facebook-github-bot