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

Don't group Idle/Offscreen work with other work #17456

Merged
merged 1 commit into from Dec 3, 2019

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Nov 26, 2019

When we suspend we always try a lower level but we shouldn't try offscreen.

If nothing pings during this time then we’ll never return back to rendering the smaller set. This leads us to overrender a lot and assume offscreen heuristics for visible content.

Offscreen really shouldn’t ever render unless we’ve committed everything else. There’s no reason to since hidden and hydrating work never unblocks anything.

This change makes it so we leave cpu cycles on the table. Ideally we could warm up hidden work and then return to the previously prepared tree but since we can’t do that yet it’s better to not throw away the tree.

@codesandbox-ci
Copy link

@codesandbox-ci codesandbox-ci bot commented Nov 26, 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 b1645e0:

Sandbox Source
React Configuration

@sizebot
Copy link

@sizebot sizebot commented Nov 26, 2019

Details of bundled changes.

Comparing: f523b2e...b1645e0

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js 0.0% +0.1% 749.83 KB 750.12 KB 158.82 KB 158.92 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% -0.0% 277.21 KB 277.21 KB 47.59 KB 47.59 KB RN_FB_PROD
ReactFabric-dev.js 0.0% +0.1% 755.24 KB 755.53 KB 159.65 KB 159.75 KB RN_OSS_DEV
ReactFabric-dev.js 0.0% +0.1% 755.42 KB 755.71 KB 159.73 KB 159.83 KB RN_FB_DEV
ReactNativeRenderer-profiling.js 0.0% 0.0% 285.75 KB 285.75 KB 49.24 KB 49.24 KB RN_OSS_PROFILING
ReactNativeRenderer-dev.js 0.0% +0.1% 750 KB 750.29 KB 158.9 KB 159 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 0.0% +0.1% 618.73 KB 619 KB 131.97 KB 132.06 KB UMD_DEV
ReactTestRenderer-dev.js 0.0% +0.1% 629.56 KB 629.85 KB 131.15 KB 131.25 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% -0.0% 39.11 KB 39.11 KB 10.01 KB 10.01 KB UMD_DEV
react-test-renderer.development.js 0.0% +0.1% 614 KB 614.27 KB 130.78 KB 130.88 KB NODE_DEV
react-test-renderer.production.min.js 0.0% 0.0% 71.52 KB 71.52 KB 21.58 KB 21.58 KB NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-dev.js +0.1% +0.1% 617.88 KB 618.2 KB 128.58 KB 128.69 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.1% 🔺+0.2% 236.02 KB 236.19 KB 39.84 KB 39.93 KB FB_WWW_PROD
react-art.development.js 0.0% +0.1% 673.82 KB 674.09 KB 146.27 KB 146.37 KB UMD_DEV
react-art.production.min.js -0.1% 0.0% 106.84 KB 106.78 KB 32.4 KB 32.41 KB UMD_PROD
react-art.development.js 0.0% +0.1% 604.46 KB 604.72 KB 128.88 KB 128.98 KB NODE_DEV
react-art.production.min.js -0.1% 0.0% 71.8 KB 71.75 KB 21.48 KB 21.48 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.0% 0.0% 123.7 KB 123.64 KB 38.8 KB 38.8 KB NODE_PROFILING
react-dom-server.browser.development.js 0.0% 0.0% 140.46 KB 140.46 KB 36.91 KB 36.91 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% 0.0% 20.39 KB 20.39 KB 7.48 KB 7.48 KB UMD_PROD
react-dom-test-utils.development.js 0.0% 0.0% 56.21 KB 56.21 KB 15.55 KB 15.55 KB UMD_DEV
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% 139.73 KB 139.78 KB 35.46 KB 35.47 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 54.48 KB 54.48 KB 15.22 KB 15.22 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 10.95 KB 10.95 KB 4.09 KB 4.09 KB NODE_PROD
react-dom.development.js 0.0% 0.0% 954.18 KB 954.45 KB 215.83 KB 215.92 KB UMD_DEV
react-dom.production.min.js -0.0% 🔺+0.1% 119.76 KB 119.7 KB 38.43 KB 38.45 KB UMD_PROD
react-dom.profiling.min.js -0.0% 0.0% 123.44 KB 123.38 KB 39.52 KB 39.53 KB UMD_PROFILING
react-dom.development.js 0.0% 0.0% 948.25 KB 948.52 KB 214.24 KB 214.34 KB NODE_DEV
react-dom-server.node.development.js 0.0% 0.0% 137.5 KB 137.5 KB 36.14 KB 36.14 KB NODE_DEV
react-dom.production.min.js -0.0% 0.0% 119.9 KB 119.84 KB 37.77 KB 37.77 KB NODE_PROD
ReactTestUtils-dev.js +0.1% +0.1% 51.28 KB 51.34 KB 13.93 KB 13.94 KB FB_WWW_DEV
react-dom-server.browser.development.js 0.0% 0.0% 136.39 KB 136.39 KB 35.91 KB 35.91 KB NODE_DEV
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.14 KB 60.14 KB 15.8 KB 15.79 KB UMD_DEV
ReactDOM-dev.js 0.0% 0.0% 976.34 KB 976.65 KB 216.76 KB 216.86 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 🔺+0.1% 402.71 KB 402.89 KB 73.42 KB 73.49 KB FB_WWW_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 59.82 KB 59.82 KB 15.66 KB 15.66 KB NODE_DEV
ReactDOM-profiling.js 0.0% +0.1% 403.64 KB 403.81 KB 73.94 KB 74.01 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10.48 KB 10.48 KB 3.57 KB 3.57 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-persistent.development.js 0.0% +0.1% 602.96 KB 603.22 KB 126.97 KB 127.07 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% 0.0% 19.09 KB 19.09 KB 6.23 KB 6.23 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% -0.1% 2.86 KB 2.86 KB 1.24 KB 1.24 KB NODE_PROD
react-reconciler.development.js 0.0% +0.1% 605.48 KB 605.75 KB 128.08 KB 128.18 KB NODE_DEV
react-reconciler.production.min.js -0.1% 0.0% 74.84 KB 74.78 KB 21.88 KB 21.88 KB NODE_PROD

ReactDOM: size: 0.0%, gzip: 0.0%

Size changes (experimental)

Generated by 🚫 dangerJS against b1645e0

@sizebot
Copy link

@sizebot sizebot commented Nov 26, 2019

Details of bundled changes.

Comparing: f523b2e...b1645e0

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js 0.0% +0.1% 673.8 KB 674.07 KB 146.27 KB 146.37 KB UMD_DEV
react-art.production.min.js 0.0% -0.0% 104.76 KB 104.76 KB 31.83 KB 31.83 KB UMD_PROD
react-art.development.js 0.0% +0.1% 604.43 KB 604.7 KB 128.87 KB 128.98 KB NODE_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js 0.0% +0.1% 749.99 KB 750.28 KB 158.89 KB 158.99 KB RN_FB_DEV
ReactFabric-dev.js 0.0% +0.1% 755.41 KB 755.7 KB 159.72 KB 159.83 KB RN_FB_DEV
ReactNativeRenderer-dev.js 0.0% +0.1% 749.81 KB 750.1 KB 158.81 KB 158.91 KB RN_OSS_DEV
ReactNativeRenderer-profiling.js 0.0% 0.0% 285.74 KB 285.74 KB 49.23 KB 49.23 KB RN_OSS_PROFILING
ReactFabric-dev.js 0.0% +0.1% 755.23 KB 755.52 KB 159.64 KB 159.74 KB RN_OSS_DEV

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js 0.0% +0.1% 618.71 KB 618.97 KB 131.96 KB 132.05 KB UMD_DEV
react-test-renderer.development.js 0.0% +0.1% 613.98 KB 614.24 KB 130.77 KB 130.87 KB NODE_DEV
react-test-renderer.production.min.js 0.0% 0.0% 71.49 KB 71.49 KB 21.56 KB 21.56 KB NODE_PROD
react-test-renderer-shallow.development.js 0.0% -0.0% 39.1 KB 39.1 KB 10 KB 10 KB UMD_DEV

Size changes (stable)

Generated by 🚫 dangerJS against b1645e0

@sebmarkbage
Copy link
Collaborator Author

@sebmarkbage sebmarkbage commented Nov 26, 2019

I think we’ll also have to do something about pingSuspendedRoot because otherwise a ping on Offscreen could start rendering at Offscreen priority while the other ones are suspended.

https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberWorkLoop.js#L2376

@sebmarkbage sebmarkbage force-pushed the trainmodel branch 2 times, most recently from b70609d to e576ea1 Compare Dec 3, 2019
nextLevel <= Idle &&
firstPendingTime !== nextLevel
) {
// Don't work on Idle/Never priority unless everything else is committed.
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Dec 3, 2019

Choose a reason for hiding this comment

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

I'm a bit uneasy about this since if something goes wrong, it's not possible to unblock by pinging these levels. However, I don't think it ever happens now because these levels wouldn't ever be suspended if something else rendered at higher pri because that makes them unsuspended. I couldn't make a test that fails.

When we suspend we always try a lower level but we shouldn't try offscreen.
Copy link
Contributor

@threepointone threepointone left a comment

lgtm!

@sebmarkbage sebmarkbage merged commit dc18b8b into facebook:master Dec 3, 2019
21 checks passed
trueadm pushed a commit to trueadm/react that referenced this issue Dec 4, 2019
When we suspend we always try a lower level but we shouldn't try offscreen.
trueadm pushed a commit to trueadm/react that referenced this issue Dec 4, 2019
When we suspend we always try a lower level but we shouldn't try offscreen.
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.

None yet

5 participants