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.

Sign up for GitHub

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
Merged

Prevent a v8 deopt when profiling #14383

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/react-reconciler/src/ReactFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,23 @@ function FiberNode(
this.alternate = null;

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

Choose a reason for hiding this comment

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

Nit: s/deopt/performance cliff/

(It's not a deopt.)

Copy link
Contributor Author

@bvaughn bvaughn Dec 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

//
// It is important to initialize the fields below with doubles.
// Otherwise Fibers will deopt and end up having separate shapes when
// doubles are later assigned to fields that initially contained smis.
// This is a bug in v8 having something to do with Object.preventExtension().
//
// Learn more about this deopt here:
// https://github.com/facebook/react/issues/14365
// https://bugs.chromium.org/p/v8/issues/detail?id=8538
this.actualDuration = Number.NaN;
this.actualStartTime = Number.NaN;
this.selfBaseDuration = Number.NaN;
this.treeBaseDuration = Number.NaN;

// It's okay to replace the initial doubles with smis after initialization.
// This simplifies other profiler code and doesn't trigger the deopt.
this.actualDuration = 0;
this.actualStartTime = -1;
this.selfBaseDuration = 0;
Expand Down