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

Prevent a v8 deopt when profiling #14383

Merged
merged 1 commit into from Dec 3, 2018

Conversation

Projects
None yet
6 participants
@bvaughn
Copy link
Contributor

bvaughn commented Dec 3, 2018

Resolves #14365

@bvaughn bvaughn requested review from sophiebits and trueadm Dec 3, 2018

@bvaughn bvaughn force-pushed the bvaughn:prevent-v8-profiler-deopt branch from 9af5dea to c5b7d26 Dec 3, 2018

@sizebot

This comment has been minimized.

Copy link

sizebot commented Dec 3, 2018

ReactDOM: size: 0.0%, gzip: -0.0%

Details of bundled changes.

Comparing: 8482cbe...c5b7d26

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 722.47 KB 723.28 KB 166.87 KB 167.11 KB UMD_DEV
react-dom.production.min.js 0.0% -0.0% 97.8 KB 97.8 KB 31.84 KB 31.84 KB UMD_PROD
react-dom.profiling.min.js +0.1% +0.1% 100.69 KB 100.78 KB 32.52 KB 32.55 KB UMD_PROFILING
react-dom.development.js +0.1% +0.1% 717.76 KB 718.58 KB 165.5 KB 165.75 KB NODE_DEV
react-dom.profiling.min.js +0.1% 0.0% 100.78 KB 100.88 KB 31.98 KB 31.99 KB NODE_PROFILING
ReactDOM-dev.js +0.1% +0.1% 739.21 KB 740.03 KB 166.51 KB 166.75 KB FB_WWW_DEV
ReactDOM-profiling.js 0.0% +0.1% 314.96 KB 315.07 KB 58.17 KB 58.21 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js +0.1% +0.1% 722.75 KB 723.57 KB 166.98 KB 167.22 KB UMD_DEV
react-dom-unstable-fire.production.min.js 0.0% -0.0% 97.81 KB 97.81 KB 31.85 KB 31.85 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js +0.1% +0.1% 100.7 KB 100.8 KB 32.53 KB 32.55 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.1% +0.2% 718.05 KB 718.86 KB 165.62 KB 165.87 KB NODE_DEV
react-dom-unstable-fire.profiling.min.js +0.1% +0.1% 100.8 KB 100.89 KB 31.98 KB 32 KB NODE_PROFILING
ReactFire-dev.js +0.1% +0.1% 738.37 KB 739.18 KB 166.45 KB 166.69 KB FB_WWW_DEV
ReactFire-profiling.js 0.0% 0.0% 303.48 KB 303.59 KB 55.8 KB 55.83 KB FB_WWW_PROFILING
react-dom-test-utils.production.min.js 0.0% 0.0% 9.74 KB 9.74 KB 3.65 KB 3.65 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.61 KB 60.61 KB 15.92 KB 15.92 KB UMD_DEV
react-dom-server.browser.development.js 0.0% 0.0% 123.82 KB 123.82 KB 33.07 KB 33.07 KB UMD_DEV
react-dom-server.browser.development.js 0.0% -0.0% 119.95 KB 119.95 KB 32.14 KB 32.14 KB NODE_DEV
ReactDOMServer-dev.js 0.0% 0.0% 121.18 KB 121.18 KB 31.79 KB 31.79 KB FB_WWW_DEV
react-dom-server.node.development.js 0.0% -0.0% 122.01 KB 122.01 KB 32.68 KB 32.68 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% 0.0% 17.77 KB 17.77 KB 6.83 KB 6.83 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% 502.87 KB 503.69 KB 110.73 KB 110.98 KB UMD_DEV
react-art.production.min.js 0.0% 0.0% 89.96 KB 89.96 KB 27.66 KB 27.66 KB UMD_PROD
react-art.development.js +0.2% +0.3% 434.61 KB 435.42 KB 93.67 KB 93.92 KB NODE_DEV
ReactART-dev.js +0.2% +0.3% 442.89 KB 443.71 KB 92.65 KB 92.89 KB FB_WWW_DEV
ReactART-prod.js 0.0% 0.0% 183.62 KB 183.62 KB 31.39 KB 31.39 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.1% +0.2% 567.6 KB 568.42 KB 123.22 KB 123.47 KB RN_FB_DEV
ReactNativeRenderer-dev.js +0.1% +0.2% 567.3 KB 568.11 KB 123.14 KB 123.39 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 224.35 KB 224.35 KB 38.96 KB 38.96 KB RN_OSS_PROD
ReactFabric-dev.js +0.1% +0.2% 558.11 KB 558.92 KB 120.79 KB 121.04 KB RN_FB_DEV
ReactFabric-dev.js +0.1% +0.2% 558.14 KB 558.96 KB 120.8 KB 121.05 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.2% +0.3% 447.31 KB 448.12 KB 96.36 KB 96.61 KB UMD_DEV
react-test-renderer.production.min.js 0.0% 0.0% 56.15 KB 56.15 KB 17.24 KB 17.24 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.2% 442.51 KB 443.32 KB 95.21 KB 95.44 KB NODE_DEV
react-test-renderer.production.min.js 0.0% -0.0% 55.82 KB 55.82 KB 17.09 KB 17.09 KB NODE_PROD
ReactTestRenderer-dev.js +0.2% +0.3% 450.9 KB 451.71 KB 94.48 KB 94.72 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% 0.0% 25.82 KB 25.82 KB 7.05 KB 7.05 KB UMD_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.2% +0.3% 432.44 KB 433.25 KB 92.17 KB 92.41 KB NODE_DEV
react-reconciler-persistent.development.js +0.2% +0.3% 430.89 KB 431.7 KB 91.54 KB 91.79 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% 🔺+0.1% 2.56 KB 2.56 KB 1.13 KB 1.13 KB NODE_PROD

Generated by 🚫 dangerJS

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Dec 3, 2018

Nice work finding that we can set them back to ints straight after. Seems like a simple fix to me. I was going to suggest just removing the preventExtensions logic, but this is better.

@trueadm

trueadm approved these changes Dec 3, 2018

@bvaughn bvaughn merged commit 7a48c90 into facebook:master Dec 3, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Dec 3, 2018

Thanks Dom! (and thanks for the help digging into this on Friday)

@bvaughn bvaughn deleted the bvaughn:prevent-v8-profiler-deopt branch Dec 3, 2018

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Dec 3, 2018

🤣

thanks for digging into this, you're both awesome

@@ -254,6 +254,23 @@ function FiberNode(
this.alternate = null;

if (enableProfilerTimer) {
// Note: The following is done to avoid a v8 deopt.

This comment has been minimized.

@mathiasbynens

mathiasbynens Dec 4, 2018

Nit: s/deopt/performance cliff/

(It's not a deopt.)

This comment has been minimized.

@bvaughn

bvaughn Dec 4, 2018

Contributor

Interesting. I was referring to the fact that the result is Fibers all having different shapes (which seemed like a deopt to me) but admittedly, I was just guessing at terminology 😄

Edit I've updated the wording based on your clarification ~ 1dc108e

This comment has been minimized.

@mathiasbynens

mathiasbynens Dec 5, 2018

To give some more background, a deopt happens when we're running optimized machine code generated by the optimizing compiler (TurboFan in V8's case), and one of the assumptions in the optimized code turns out to be invalid. In such cases we have to deopt and go back to running bytecode in the interpreter (Ignition in V8's case). (Our JSConf talk includes some more detail and visualizations that make this more clear, in case you're interested.)

This V8 bug however occurs before we even get to optimized code.

You can verify this yourself by running our reduced test case with --no-opt: it still reproduces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment