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

Remove renderPhaseUpdates Map #17484

merged 3 commits into from Dec 14, 2019

Conversation

@sebmarkbage
Copy link
Member

sebmarkbage commented Nov 28, 2019

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

This comment has been minimized.

Copy link

codesandbox 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

This comment has been minimized.

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

This comment has been minimized.

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 sebmarkbage force-pushed the sebmarkbage:hook-rerender branch from e2a1384 to 3f3c8b4 Nov 28, 2019
@sebmarkbage

This comment has been minimized.

Copy link
Member Author

sebmarkbage commented Dec 3, 2019

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

@sebmarkbage sebmarkbage force-pushed the sebmarkbage:hook-rerender branch from 3f3c8b4 to 0f85d01 Dec 3, 2019
@acdlite

This comment has been minimized.

Copy link
Member

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.

@acdlite acdlite force-pushed the sebmarkbage:hook-rerender branch from 0f85d01 to 6388040 Dec 13, 2019
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
21 checks passed
21 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_experimental Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts_experimental Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: sizebot Your tests passed on CircleCI!
Details
ci/circleci: sizebot_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
ci/circleci: test_build_devtools Your tests passed on CircleCI!
Details
ci/circleci: test_build_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: test_source Your tests passed on CircleCI!
Details
ci/circleci: test_source_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_source_persistent Your tests passed on CircleCI!
Details
ci/circleci: test_source_prod Your tests passed on CircleCI!
Details
ci/codesandbox Building packages succeeded.
Details
@acdlite

This comment has been minimized.

Copy link
Member

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
This reverts commit 8a347ed.
gaearon added a commit that referenced this pull request Dec 16, 2019
This reverts commit 8a347ed.
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;

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Dec 16, 2019

Author Member

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

This comment has been minimized.

Copy link
@acdlite

acdlite Dec 16, 2019

Member

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 added 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
4 participants
You can’t perform that action at this time.