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

Clear finished discrete updates during commit phase #18515

Merged
merged 4 commits into from
Apr 7, 2020

Conversation

jddxf
Copy link
Contributor

@jddxf jddxf commented Apr 7, 2020

Fix #18020

If a root is finished at a priority lower than that of the latest pending discrete updates on it, these updates must have been finished so we can clear them now. Otherwise, a later call of flushDiscreteUpdates would start a new empty render pass which may cause a scheduled timeout to be cancelled.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 7, 2020

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 6f67d0a:

Sandbox Source
fervent-franklin-08kn6 Configuration
dreamy-morning-ecdx5 Issue #18020

@sizebot
Copy link

sizebot commented Apr 7, 2020

Details of bundled changes.

Comparing: bce982b...6f67d0a

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 119.45 KB 119.61 KB 37.36 KB 37.41 KB NODE_PROD
react-dom-test-utils.development.js 0.0% 0.0% 75.12 KB 75.12 KB 20.1 KB 20.11 KB UMD_DEV
ReactDOMTesting-profiling.js +0.1% +0.1% 403.76 KB 404.28 KB 73.07 KB 73.15 KB FB_WWW_PROFILING
react-dom.profiling.min.js +0.1% +0.1% 123.31 KB 123.47 KB 38.54 KB 38.59 KB NODE_PROFILING
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.19 KB 1.19 KB 698 B 697 B UMD_PROD
ReactDOMForked-dev.js +0.1% +0.2% 999.58 KB 1000.83 KB 221.33 KB 221.68 KB FB_WWW_DEV
ReactDOMForked-prod.js 🔺+0.1% 🔺+0.1% 420.13 KB 420.53 KB 75.41 KB 75.47 KB FB_WWW_PROD
react-dom.development.js +0.1% +0.2% 900.63 KB 901.9 KB 197.75 KB 198.08 KB UMD_DEV
ReactDOMForked-profiling.js +0.1% +0.1% 431 KB 431.41 KB 77.28 KB 77.35 KB FB_WWW_PROFILING
react-dom-server.browser.development.js 0.0% -0.0% 154.55 KB 154.55 KB 39.36 KB 39.36 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 119.33 KB 119.49 KB 38.16 KB 38.21 KB UMD_PROD
react-dom-server.browser.production.min.js 0.0% 0.0% 22.85 KB 22.85 KB 8.48 KB 8.49 KB UMD_PROD
react-dom.profiling.min.js +0.1% +0.1% 123.05 KB 123.2 KB 39.36 KB 39.41 KB UMD_PROFILING
ReactDOMTesting-dev.js +0.1% +0.2% 951.76 KB 953.02 KB 211.56 KB 211.9 KB FB_WWW_DEV
react-dom.development.js +0.1% +0.2% 857.37 KB 858.6 KB 195.29 KB 195.62 KB NODE_DEV
ReactDOMTesting-prod.js 🔺+0.1% 🔺+0.1% 403.76 KB 404.28 KB 73.07 KB 73.15 KB FB_WWW_PROD
ReactDOM-dev.js +0.1% +0.2% 999.58 KB 1000.83 KB 221.33 KB 221.68 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 420.13 KB 420.53 KB 75.41 KB 75.47 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% 0.0% 69.97 KB 69.97 KB 19.6 KB 19.6 KB NODE_DEV
ReactDOM-profiling.js +0.1% +0.1% 431 KB 431.41 KB 77.28 KB 77.35 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 56.1 KB 56.1 KB 13.84 KB 13.84 KB UMD_DEV
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 53.2 KB 53.2 KB 13.64 KB 13.64 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 9.73 KB 9.73 KB 3.25 KB 3.25 KB NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% +0.2% 627.94 KB 629.22 KB 132.31 KB 132.63 KB UMD_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.2% 106.9 KB 107.05 KB 32.35 KB 32.4 KB UMD_PROD
react-art.development.js +0.2% +0.3% 532.54 KB 533.78 KB 114.67 KB 115.02 KB NODE_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.3% 71.93 KB 72.09 KB 21.55 KB 21.61 KB NODE_PROD
ReactART-dev.js +0.2% +0.3% 594 KB 595.25 KB 124.39 KB 124.77 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.2% 🔺+0.2% 247.89 KB 248.35 KB 41.79 KB 41.86 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.2% +0.3% 565.92 KB 567.19 KB 117.59 KB 117.93 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.2% 🔺+0.3% 73.96 KB 74.12 KB 22.43 KB 22.49 KB UMD_PROD
react-test-renderer-shallow.production.min.js 0.0% 0.0% 12.73 KB 12.73 KB 3.96 KB 3.97 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.3% 539.57 KB 540.8 KB 116.22 KB 116.59 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.2% 🔺+0.2% 73.71 KB 73.87 KB 22.05 KB 22.1 KB NODE_PROD
ReactTestRenderer-dev.js +0.2% +0.3% 573.3 KB 574.55 KB 120.8 KB 121.15 KB FB_WWW_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.3% 651.33 KB 652.58 KB 139.7 KB 140.05 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.2% 🔺+0.2% 270.38 KB 270.91 KB 46.42 KB 46.49 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.2% +0.2% 282.28 KB 282.74 KB 48.62 KB 48.69 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.2% +0.3% 633.67 KB 634.92 KB 135.38 KB 135.74 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.2% 🔺+0.2% 262.52 KB 263.05 KB 44.94 KB 45.01 KB RN_OSS_PROD
ReactFabric-profiling.js +0.2% +0.2% 274.42 KB 274.89 KB 47.16 KB 47.24 KB RN_OSS_PROFILING

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.2% +0.3% 571.91 KB 573.14 KB 120.85 KB 121.21 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.2% 🔺+0.3% 76.94 KB 77.09 KB 22.69 KB 22.76 KB NODE_PROD
react-reconciler-reflection.production.min.js 0.0% 🔺+0.1% 2.57 KB 2.57 KB 1.08 KB 1.08 KB NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 6f67d0a

@sizebot
Copy link

sizebot commented Apr 7, 2020

Details of bundled changes.

Comparing: bce982b...6f67d0a

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% +0.2% 648.22 KB 649.49 KB 136 KB 136.33 KB UMD_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.1% 109.51 KB 109.66 KB 33.01 KB 33.06 KB UMD_PROD
react-art.development.js +0.2% +0.3% 552.02 KB 553.26 KB 118.44 KB 118.79 KB NODE_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.2% 74.48 KB 74.64 KB 22.19 KB 22.24 KB NODE_PROD
ReactART-dev.js +0.2% +0.3% 583.94 KB 585.19 KB 122.4 KB 122.78 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.2% 🔺+0.2% 240.29 KB 240.78 KB 40.52 KB 40.6 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-prod.js 🔺+0.2% 🔺+0.2% 270.39 KB 270.92 KB 46.42 KB 46.5 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.2% +0.2% 282.29 KB 282.76 KB 48.63 KB 48.7 KB RN_OSS_PROFILING
ReactFabric-prod.js 🔺+0.2% 🔺+0.2% 262.53 KB 263.07 KB 44.94 KB 45.02 KB RN_OSS_PROD
ReactFabric-profiling.js +0.2% +0.2% 274.44 KB 274.9 KB 47.17 KB 47.24 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.2% +0.3% 636.1 KB 637.35 KB 135.7 KB 136.05 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.2% 🔺+0.2% 262.68 KB 263.22 KB 44.98 KB 45.05 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.2% +0.3% 651.35 KB 652.6 KB 139.7 KB 140.06 KB RN_OSS_DEV
ReactFabric-profiling.js +0.2% +0.2% 274.58 KB 275.05 KB 47.19 KB 47.27 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.2% +0.3% 653.75 KB 655 KB 140.03 KB 140.38 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.2% 🔺+0.2% 270.53 KB 271.07 KB 46.46 KB 46.53 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.2% +0.2% 282.44 KB 282.9 KB 48.65 KB 48.73 KB RN_FB_PROFILING
ReactFabric-dev.js +0.2% +0.3% 633.69 KB 634.94 KB 135.39 KB 135.74 KB RN_OSS_DEV

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactTestRenderer-dev.js +0.2% +0.3% 573.32 KB 574.57 KB 120.81 KB 121.16 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% 0.0% 38.72 KB 38.72 KB 9.42 KB 9.42 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 12.74 KB 12.74 KB 3.97 KB 3.97 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.3% 565.94 KB 567.22 KB 117.61 KB 117.94 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.2% 🔺+0.3% 73.99 KB 74.15 KB 22.45 KB 22.51 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.3% 539.59 KB 540.83 KB 116.23 KB 116.6 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.2% 🔺+0.2% 73.73 KB 73.89 KB 22.06 KB 22.12 KB NODE_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-server.browser.production.min.js 0.0% 0.0% 23.32 KB 23.32 KB 8.57 KB 8.57 KB UMD_PROD
react-dom.profiling.min.js +0.1% +0.1% 127.58 KB 127.74 KB 39.76 KB 39.81 KB NODE_PROFILING
ReactDOM-dev.js +0.1% +0.2% 971.7 KB 972.95 KB 215.32 KB 215.66 KB FB_WWW_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.2 KB 1.2 KB 706 B 705 B UMD_PROD
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 407.07 KB 407.5 KB 73.51 KB 73.58 KB FB_WWW_PROD
ReactDOM-profiling.js +0.1% +0.1% 417.88 KB 418.32 KB 75.34 KB 75.41 KB FB_WWW_PROFILING
react-dom-test-utils.development.js 0.0% 0.0% 75.14 KB 75.14 KB 20.11 KB 20.12 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 13.23 KB 13.23 KB 4.9 KB 4.9 KB UMD_PROD
ReactDOMTesting-dev.js +0.1% +0.2% 925.22 KB 926.47 KB 205.96 KB 206.3 KB FB_WWW_DEV
ReactDOMTesting-prod.js 🔺+0.1% 🔺+0.1% 391.79 KB 392.33 KB 71.27 KB 71.36 KB FB_WWW_PROD
ReactDOMTesting-profiling.js +0.1% +0.1% 391.79 KB 392.33 KB 71.27 KB 71.36 KB FB_WWW_PROFILING
react-dom-server.node.production.min.js 0.0% 0.0% 23.62 KB 23.62 KB 8.7 KB 8.7 KB NODE_PROD
react-dom-server.browser.production.min.js 0.0% 0.0% 23.21 KB 23.21 KB 8.54 KB 8.54 KB NODE_PROD
react-dom.development.js +0.1% +0.2% 930.45 KB 931.73 KB 203.03 KB 203.37 KB UMD_DEV
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 56.11 KB 56.11 KB 13.85 KB 13.85 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 123.41 KB 123.57 KB 39.3 KB 39.35 KB UMD_PROD
ReactDOMForked-dev.js +0.1% +0.2% 971.7 KB 972.95 KB 215.32 KB 215.66 KB FB_WWW_DEV
ReactDOMServer-dev.js 0.0% -0.0% 153.06 KB 153.06 KB 38.75 KB 38.75 KB FB_WWW_DEV
react-dom.profiling.min.js +0.1% +0.1% 127.23 KB 127.39 KB 40.55 KB 40.59 KB UMD_PROFILING
ReactDOMForked-prod.js 🔺+0.1% 🔺+0.1% 407.07 KB 407.5 KB 73.51 KB 73.58 KB FB_WWW_PROD
react-dom.development.js +0.1% +0.2% 885.96 KB 887.19 KB 200.54 KB 200.89 KB NODE_DEV
ReactDOMForked-profiling.js +0.1% +0.1% 417.88 KB 418.32 KB 75.34 KB 75.41 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 53.21 KB 53.21 KB 13.65 KB 13.65 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 5.18 KB 5.18 KB 1.7 KB 1.7 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 123.6 KB 123.76 KB 38.52 KB 38.57 KB NODE_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 9.75 KB 9.75 KB 3.25 KB 3.26 KB NODE_PROD
react-dom-unstable-fizz.node.production.min.js 0.0% -0.1% 1.17 KB 1.17 KB 668 B 667 B NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-reflection.development.js 0.0% 0.0% 16.22 KB 16.22 KB 4.9 KB 4.91 KB NODE_DEV
react-reconciler.development.js +0.2% +0.3% 593.75 KB 594.98 KB 125.12 KB 125.48 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.2% 🔺+0.3% 79.9 KB 80.06 KB 23.43 KB 23.49 KB NODE_PROD

ReactDOM: size: 0.0%, gzip: 0.0%

Size changes (experimental)

Generated by 🚫 dangerJS against 6f67d0a

@gaearon
Copy link
Collaborator

gaearon commented Apr 7, 2020

I'm not super familiar with this code but it seems a bit fragile. Are we treating the symptom or the cause?

You said:

And no timeout would be scheduled again for some reason. Thus the placeholder is never showed.

Do we understand why?

@acdlite
Copy link
Collaborator

acdlite commented Apr 7, 2020

Thanks for the unit test!

The underlying issue is that we're clearing the timeout without making sure the priority level that corresponds to that level is rescheduled.

It's usually fine to clear a pending timeout at the beginning of a new render (in prepareFreshStack) because when we receive an update at a certain priority level, we mark that level and anything lower as "unsuspended." Which means we'll attempt to render the pending level again after the high pri render has finished:

// Update the range of suspended times. Treat everything lower priority or
// equal to this update as unsuspended.
const firstSuspendedTime = root.firstSuspendedTime;
if (firstSuspendedTime !== NoWork) {
if (expirationTime >= firstSuspendedTime) {
// The entire suspended range is now unsuspended.
root.firstSuspendedTime = root.lastSuspendedTime = root.nextKnownPendingLevel = NoWork;
} else if (expirationTime >= root.lastSuspendedTime) {
root.lastSuspendedTime = expirationTime + 1;
}

However, flushDiscreteUpdates is a case where we start rendering without having received a new update or ping.

There are multiple ways we could fix this. My instinct is to fix it inside prepareFreshStack. We should check that the suspended priority level is marked in such a way that we will return to it later. Probably by marking it as pinged.

We should also commit your changes here, but let's fix the underlying issue first. I'll take a closer look at this.

acdlite and others added 2 commits April 7, 2020 13:11
Make sure the suspended level is marked as pinged so that we return back
to it later, in case the render we're about to start gets aborted.
Generally we only reach this path via a ping, but we shouldn't assume
that will always be the case.
If a root is finished at a priority lower than that of the latest pending discrete
updates on it, these updates must have been finished so we can clear them now.
Otherwise, a later call of `flushDiscreteUpdates` would start a new empty render
pass which may cause a scheduled timeout to be cancelled.
Happened to find this while writing a test. A JSX element comparison
failed because one of them elements had a functional component as an
owner, which should ever happen.

I'll add a regression test later.
@acdlite acdlite merged commit ddc4b65 into facebook:master Apr 7, 2020
@acdlite
Copy link
Collaborator

acdlite commented Apr 7, 2020

Thanks again for the fix! 🎉

acdlite added a commit to acdlite/react that referenced this pull request Apr 8, 2020
Adds a regression test for the same underlying bug as facebook#18515 but using
pings.

Test already passes, but I confirmed it fails if you revert the fix
in facebook#18515.
acdlite added a commit that referenced this pull request Apr 8, 2020
* Add another test for #18515 using pings

Adds a regression test for the same underlying bug as #18515 but using
pings.

Test already passes, but I confirmed it fails if you revert the fix
in #18515.

* Set nextPendingLevel after commit, too
@jddxf jddxf deleted the lost-fallback branch April 8, 2020 05:15
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.

Bug: event.preventDefault is wrecking havoc with startTransition
5 participants