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

Remove renderPhaseUpdates Map #17484

Merged
merged 3 commits into from
Dec 14, 2019
Merged

Conversation

sebmarkbage
Copy link
Collaborator

Builds on top of #17483

Now that we have a separate queue for recently added updates, we can use this queue to pick up the render phase updates. As long as we clear them before committing.

TODO: I believe this might have a bug if you suspend during the second update but before we get to process this particular Hook. Since the update would remain in the queue in this case.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 28, 2019

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

Sandbox Source
nameless-sun-3qy43 Configuration

@sizebot
Copy link

sizebot commented Nov 28, 2019

Details of bundled changes.

Comparing: be603f5...b384dd1

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.production.min.js 🔺+0.3% 0.0% 71.94 KB 72.19 KB 21.29 KB 21.3 KB NODE_PROD
react-reconciler-persistent.development.js +1.1% +0.1% 599.87 KB 606.33 KB 126.77 KB 126.92 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.3% 0.0% 71.95 KB 72.2 KB 21.3 KB 21.31 KB NODE_PROD
react-reconciler.development.js +1.1% +0.1% 602.4 KB 608.86 KB 127.89 KB 128.03 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% 0.0% 20.01 KB 20.01 KB 6.42 KB 6.42 KB NODE_DEV

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.71 KB 10.71 KB 3.63 KB 3.63 KB NODE_PROD
react-dom.development.js +0.7% +0.1% 950.96 KB 957.42 KB 215.62 KB 215.76 KB UMD_DEV
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.87 KB 3.87 KB 1.54 KB 1.54 KB UMD_DEV
react-dom.production.min.js 🔺+0.2% 🔺+0.2% 115.77 KB 116.01 KB 37.46 KB 37.52 KB UMD_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.2 KB 1.2 KB 701 B 702 B UMD_PROD
react-dom.profiling.min.js +0.2% +0.3% 119.35 KB 119.58 KB 38.54 KB 38.64 KB UMD_PROFILING
react-dom.development.js +0.7% +0.1% 945.02 KB 951.49 KB 214.04 KB 214.19 KB NODE_DEV
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.7 KB 3.7 KB 1.49 KB 1.5 KB NODE_DEV
react-dom.production.min.js 🔺+0.2% -0.0% 115.88 KB 116.13 KB 36.87 KB 36.85 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 61.56 KB 61.56 KB 16.11 KB 16.11 KB UMD_DEV
react-dom.profiling.min.js +0.2% 0.0% 119.58 KB 119.83 KB 37.83 KB 37.84 KB NODE_PROFILING
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.95 KB 10.95 KB 3.72 KB 3.72 KB UMD_PROD
react-dom-server.node.development.js 0.0% 0.0% 138.36 KB 138.36 KB 36.19 KB 36.19 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 11.17 KB 11.17 KB 4.14 KB 4.14 KB UMD_PROD
react-dom-test-utils.development.js 0.0% 0.0% 56.4 KB 56.4 KB 15.41 KB 15.41 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 10.94 KB 10.94 KB 4.08 KB 4.08 KB NODE_PROD
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.3% 1.2 KB 1.2 KB 688 B 690 B NODE_PROD
react-dom-server.browser.production.min.js 0.0% 0.0% 19.93 KB 19.93 KB 7.39 KB 7.39 KB UMD_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +1.0% +0.1% 670.75 KB 677.21 KB 146.08 KB 146.23 KB UMD_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.2% 104.17 KB 104.41 KB 31.83 KB 31.89 KB UMD_PROD
react-art.development.js +1.1% +0.1% 601.39 KB 607.86 KB 128.66 KB 128.8 KB NODE_DEV
react-art.production.min.js 🔺+0.3% 🔺+0.1% 69.18 KB 69.42 KB 20.98 KB 21 KB NODE_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +1.1% +0.1% 615.62 KB 622.08 KB 131.74 KB 131.89 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.3% 71.19 KB 71.44 KB 21.9 KB 21.96 KB UMD_PROD
react-test-renderer.development.js +1.1% +0.1% 610.88 KB 617.35 KB 130.55 KB 130.7 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.1% 70.9 KB 71.14 KB 21.53 KB 21.55 KB NODE_PROD
react-test-renderer-shallow.development.js 0.0% -0.0% 39.18 KB 39.18 KB 10.06 KB 10.06 KB UMD_DEV
react-test-renderer-shallow.development.js 0.0% 0.0% 33.14 KB 33.14 KB 8.58 KB 8.58 KB NODE_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.0% 11.72 KB 11.72 KB 3.68 KB 3.68 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.9% +0.1% 743.7 KB 750.12 KB 158.15 KB 158.31 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.2% 276.52 KB 276.76 KB 47.7 KB 47.81 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.1% +0.2% 285.45 KB 285.69 KB 49.43 KB 49.52 KB RN_FB_PROFILING
ReactFabric-dev.js +0.9% +0.1% 748.73 KB 755.16 KB 158.92 KB 159.06 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.1% 🔺+0.2% 268.18 KB 268.42 KB 46.32 KB 46.43 KB RN_FB_PROD
ReactFabric-profiling.js +0.1% +0.3% 278.28 KB 278.53 KB 48.17 KB 48.29 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.9% +0.1% 743.52 KB 749.95 KB 158.06 KB 158.21 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.2% 276.13 KB 276.37 KB 47.63 KB 47.73 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.1% +0.2% 285.06 KB 285.31 KB 49.35 KB 49.45 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.9% +0.1% 748.55 KB 754.98 KB 158.83 KB 158.98 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.1% 🔺+0.2% 267.83 KB 268.07 KB 46.24 KB 46.35 KB RN_OSS_PROD
ReactFabric-profiling.js +0.1% +0.3% 277.94 KB 278.18 KB 48.09 KB 48.21 KB RN_OSS_PROFILING

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

Size changes (stable)

Generated by 🚫 dangerJS against b384dd1

@sizebot
Copy link

sizebot commented Nov 28, 2019

Details of bundled changes.

Comparing: be603f5...b384dd1

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFabric-prod.js 🔺+0.1% 🔺+0.2% 268.19 KB 268.43 KB 46.33 KB 46.44 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.9% +0.1% 743.53 KB 749.96 KB 158.06 KB 158.22 KB RN_OSS_DEV
ReactFabric-profiling.js +0.1% +0.3% 278.29 KB 278.54 KB 48.18 KB 48.3 KB RN_FB_PROFILING
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.2% 276.53 KB 276.78 KB 47.71 KB 47.82 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.1% +0.2% 285.46 KB 285.71 KB 49.44 KB 49.53 KB RN_FB_PROFILING
ReactFabric-dev.js +0.9% +0.1% 748.56 KB 754.99 KB 158.84 KB 158.99 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.1% 🔺+0.2% 267.84 KB 268.08 KB 46.25 KB 46.35 KB RN_OSS_PROD
ReactFabric-profiling.js +0.1% +0.3% 277.95 KB 278.19 KB 48.1 KB 48.22 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.9% +0.1% 748.75 KB 755.17 KB 158.92 KB 159.07 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.2% 276.14 KB 276.39 KB 47.64 KB 47.74 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.1% +0.2% 285.08 KB 285.32 KB 49.36 KB 49.45 KB RN_OSS_PROFILING
ReactNativeRenderer-dev.js +0.9% +0.1% 743.71 KB 750.14 KB 158.16 KB 158.31 KB RN_FB_DEV

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +1.1% +0.1% 615.64 KB 622.11 KB 131.76 KB 131.9 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.3% 71.22 KB 71.46 KB 21.91 KB 21.98 KB UMD_PROD
ReactTestRenderer-dev.js +1.0% +0.1% 624.44 KB 630.9 KB 130.89 KB 131.04 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% -0.0% 39.2 KB 39.2 KB 10.07 KB 10.07 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.0% 11.73 KB 11.73 KB 3.69 KB 3.69 KB NODE_PROD
react-test-renderer.development.js +1.1% +0.1% 610.91 KB 617.38 KB 130.56 KB 130.71 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.1% 70.93 KB 71.17 KB 21.55 KB 21.57 KB NODE_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js +0.2% -0.0% 123.08 KB 123.32 KB 38.8 KB 38.8 KB NODE_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.88 KB 3.88 KB 1.55 KB 1.55 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 11.18 KB 11.18 KB 4.15 KB 4.15 KB UMD_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.21 KB 1.21 KB 709 B 710 B UMD_PROD
ReactDOMServer-dev.js 0.0% 0.0% 141.04 KB 141.04 KB 35.54 KB 35.54 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 56.42 KB 56.42 KB 15.42 KB 15.42 KB NODE_DEV
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.71 KB 3.71 KB 1.5 KB 1.5 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 10.95 KB 10.95 KB 4.09 KB 4.09 KB NODE_PROD
react-dom.development.js +0.7% +0.1% 950.98 KB 957.44 KB 215.64 KB 215.78 KB UMD_DEV
react-dom.production.min.js 🔺+0.2% 0.0% 119.12 KB 119.36 KB 38.43 KB 38.45 KB UMD_PROD
react-dom.profiling.min.js +0.2% +0.4% 122.81 KB 123.05 KB 39.49 KB 39.65 KB UMD_PROFILING
react-dom.development.js +0.7% +0.1% 945.04 KB 951.51 KB 214.06 KB 214.21 KB NODE_DEV
react-dom-server.node.development.js 0.0% 0.0% 138.38 KB 138.38 KB 36.19 KB 36.19 KB NODE_DEV
react-dom.production.min.js 🔺+0.2% -0.1% 119.27 KB 119.51 KB 37.81 KB 37.78 KB NODE_PROD
react-dom-server.node.production.min.js 0.0% -0.0% 20.73 KB 20.73 KB 7.61 KB 7.61 KB NODE_PROD
react-dom-server.browser.production.min.js 0.0% -0.0% 20.32 KB 20.32 KB 7.46 KB 7.46 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 61.58 KB 61.58 KB 16.12 KB 16.12 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 10.96 KB 10.96 KB 3.73 KB 3.73 KB UMD_PROD
ReactDOM-dev.js +0.7% +0.1% 970.76 KB 977.22 KB 216.47 KB 216.62 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 🔺+0.1% 402.74 KB 402.91 KB 73.55 KB 73.65 KB FB_WWW_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 61.28 KB 61.28 KB 16.04 KB 16.04 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 4.42 KB 4.42 KB 1.65 KB 1.65 KB NODE_DEV
ReactDOM-profiling.js 0.0% +0.1% 403.66 KB 403.84 KB 74.11 KB 74.22 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.73 KB 10.73 KB 3.64 KB 3.64 KB NODE_PROD
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.3% 1.21 KB 1.21 KB 696 B 698 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-dev.js +1.1% +0.1% 613 KB 619.46 KB 128.36 KB 128.5 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.1% 🔺+0.2% 235.58 KB 235.76 KB 40.01 KB 40.1 KB FB_WWW_PROD
react-art.development.js +1.0% +0.1% 670.77 KB 677.23 KB 146.08 KB 146.23 KB UMD_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.4% 106.19 KB 106.43 KB 32.35 KB 32.48 KB UMD_PROD
react-art.development.js +1.1% +0.1% 601.42 KB 607.88 KB 128.67 KB 128.81 KB NODE_DEV
react-art.production.min.js 🔺+0.3% -0.1% 71.16 KB 71.4 KB 21.5 KB 21.47 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-persistent.development.js +1.1% +0.1% 599.88 KB 606.34 KB 126.78 KB 126.93 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% 0.0% 20.02 KB 20.02 KB 6.42 KB 6.43 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.3% 0.0% 71.97 KB 72.22 KB 21.31 KB 21.31 KB NODE_PROD
react-reconciler.development.js +1.1% +0.1% 602.41 KB 608.87 KB 127.9 KB 128.03 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.3% 🔺+0.1% 74.18 KB 74.43 KB 21.87 KB 21.89 KB NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against b384dd1

@sebmarkbage
Copy link
Collaborator Author

I should also make the infinite loop error use a dispatcher: #17480 (comment)

@acdlite
Copy link
Collaborator

acdlite commented Dec 13, 2019

TODO: I believe this might have a bug if you suspend during the second update but before we get to process this particular Hook. Since the update would remain in the queue in this case.

Inside resetHooks, which is called when something throws, scan through the partial list of work-in-progress hooks and clear the pending updates (queue.pending = null). We only need to do this if a re-render was scheduled.

When aborting a render, we also need to throw out render phase updates.
Remove the updates from the queues so they do not persist to the next
render. We already did a single pass through the whole list of hooks, so
we know that any pending updates must have been dispatched during the
render phase. The ones that were dispatched before we started rendering
were already transferred to the current hook's queue.
@acdlite acdlite merged commit 8a347ed into facebook:master Dec 14, 2019
@acdlite
Copy link
Collaborator

acdlite commented Dec 14, 2019

Fixed the todo and merged after discussing out of band.

gaearon added a commit that referenced this pull request Dec 16, 2019
gaearon added a commit that referenced this pull request Dec 16, 2019
acdlite added a commit to acdlite/react that referenced this pull request Dec 16, 2019
Follow up to facebook#17484, which was reverted due to a bug found in www.
// It's also called inside mountIndeterminateComponent if we determine the
// component is a module-style component.
if (didScheduleRenderPhaseUpdate) {
const current = (currentlyRenderingFiber: any).alternate;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acdlite What if this is render phase updates during initial mount?

Copy link
Collaborator

@acdlite acdlite Dec 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter then because either the whole fiber will get thrown out or it will be cloned again. Except in legacy mode goddamn it. I'll add a test in the other PR.

acdlite added a commit to acdlite/react that referenced this pull request Dec 19, 2019
Follow up to facebook#17484, which was reverted due to a bug found in www.
smagill pushed a commit to smagill/react that referenced this pull request Dec 20, 2019
Follow up to facebook#17484, which was reverted due to a bug found in www.
acdlite added a commit to acdlite/react that referenced this pull request Jan 9, 2020
Follow up to facebook#17484, which was reverted due to a bug found in www.
acdlite added a commit to acdlite/react that referenced this pull request Jan 9, 2020
Follow up to facebook#17484, which was reverted due to a bug found in www.
acdlite added a commit to acdlite/react that referenced this pull request Jan 17, 2020
Follow up to facebook#17484, which was reverted due to a bug found in www.
acdlite added a commit that referenced this pull request Jan 17, 2020
* Remove renderPhaseUpdates Map

Follow up to #17484, which was reverted due to a bug found in www.

* Failing test: Dropped updates

When resetting render phase updates after a throw, we should only clear
the pending queue of hooks that were already processed.

* Fix non-render-phase updates being dropped

Detects if a queue has been processed by whether the hook was cloned.
If we change the implementation to an array instead of a list, we'll
need some other mechanism to determine whether the hook was processed.

* Regression test: startTransition in render phase

useTransition uses the state hook as part of its implementation, so we
need to fork it in the dispatcher used for re-renders, too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants