Possible v8 de-opt for profiling react-dom bundles

[bvaughn]

I've observed that when React's profiling mode is enabled, updating the profiling fields here and here during the render phase seems to cause a significant deopt in getHostSibling during the commit phase for Chrome only. (Neither Safari nor Firefox seem to be affected by this.)

I have created a repro case with inline annotations here:
https://github.com/bvaughn/react-profiling-v8-deopt-repro#about-this-repro-case

(Note that the above repo is private. Please tag me here if you need access.)

[bvaughn]

cc @bmeurer and @mathiasbynens

[mathiasbynens]

@bvaughn Can haz repo access? I imagine @bmeurer would want access as well.

[bvaughn]

Yes, sorry @mathiasbynens. I jumped the gun by creating this issue and tagging you before I remembered that I needed to check with @uriklar for permission before sharing his part of the repro case with you. Hopefully I'll get confirmation soon and will leave a comment here when I do.

[mathiasbynens]

I mean, the more minimal the repro, the better.

[bvaughn]

Understood. What I meant was that he provided the repro case and I tracked it down and added the annotations to react-dom. I'm sure it will be okay to share, I just want to confirm first. Sorry 🙁

[trueadm]

I dug more into this tonight and found that the issue seemed to be around the fact the the properties being set were double values. Given that scheduler.unstable_now() is essentially a wrapper around performance.now() and returns a double value, I changed scheduler.unstable_now() so that it returned an int (specifically, Math.round(performance.now() * 1000) to make it an int, but that also means changing the logic later on to divide by 1000 again). Upon doing so, the performance issue goes away.

This definitely seems like a bug in V8, as using any other browser does not exhibit the same performance issues. Could having double properties on the fiber object really be causing deopts? I didn't have time to setup D8 and run profView, I can get around to this next week when I'm back in the office.

[mathiasbynens]

I didn't have time to setup D8 and run profView

Note that jsvu makes it trivial to get a d8 binary with --prof functionality.

[uriklar]

Hi @bvaughn , It's ok to invite any individuals you need to the repo. But if you want to make it public I would need to change the data in it..

EDIT: I see the repo contains some git history of files I removed from the original project to create the minimal repo. Before granting anyone access I would need you (or I can do it too) to create a fresh repo with the files but no git history.

Sorry about making a git soup... :-/

[mathiasbynens]

It’s okay, we can wait for a reduced test case.

[uriklar]

Ok i just removed the git history from Brian's repo so he can give you access now

[bvaughn]

Thanks @uriklar!

@bmeurer and @mathiasbynens, you should have access now. Please let me know if anything doesn't make sense.

[mathiasbynens]

Alright, we’ll take a look after the weekend :)

[bmeurer]

Looking into this with @mathiasbynens today, we found that the root cause is somehow related to double field related instance migrations combined with Object.preventExtension(). With this combination, all FiberNodes end up having separate shapes at some point, obviously making things like getHostSibling() super slow.

The work-around for that on your side is to make sure to either have the time values represented as Smis consistently as @trueadm suggested, or make sure to initialize them to double values from the beginning, i.e. use something like

diff --git a/vendor/react-dom.development.js b/vendor/react-dom.development.js
index 94b4662..78137cd 100644
--- a/vendor/react-dom.development.js
+++ b/vendor/react-dom.development.js
@@ -9765,10 +9765,10 @@ function FiberNode(tag, pendingProps, key, mode) {
   this.alternate = null;

   if (enableProfilerTimer) {
-    this.actualDuration = 0;
-    this.actualStartTime = -1;
-    this.selfBaseDuration = 0;
-    this.treeBaseDuration = 0;
+    this.actualDuration = Number.MIN_VALUE;
+    this.actualStartTime = -1.1;
+    this.selfBaseDuration = Number.MIN_VALUE;
+    this.treeBaseDuration = Number.MIN_VALUE;
   }

   {

to make sure the fields are all initialized to doubles from the beginning (as a cleaner fix I'd suggest to use NaN for at least actualStartTime, maybe all of them).

[trueadm]

@bmeurer @mathiasbynens A huge thanks to both of you for looking into this issue! :)