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

Possible v8 de-opt for profiling react-dom bundles #14365

Closed
bvaughn opened this Issue Nov 30, 2018 · 29 comments

Comments

Projects
None yet
6 participants
@bvaughn
Copy link
Contributor

bvaughn commented Nov 30, 2018

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 bvaughn changed the title Possible v8 de-opt [WIP] Possible v8 de-opt for profiling react-dom bundles Nov 30, 2018

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Nov 30, 2018

@mathiasbynens

This comment has been minimized.

Copy link

mathiasbynens commented Nov 30, 2018

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

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Nov 30, 2018

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

This comment has been minimized.

Copy link

mathiasbynens commented Nov 30, 2018

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

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Nov 30, 2018

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

This comment has been minimized.

Copy link
Contributor

trueadm commented Dec 1, 2018

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

This comment has been minimized.

Copy link

mathiasbynens commented Dec 1, 2018

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

This comment has been minimized.

Copy link

uriklar commented Dec 1, 2018

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

This comment has been minimized.

Copy link

mathiasbynens commented Dec 1, 2018

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

@uriklar

This comment has been minimized.

Copy link

uriklar commented Dec 1, 2018

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

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Dec 1, 2018

Thanks @uriklar!

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

@mathiasbynens

This comment has been minimized.

Copy link

mathiasbynens commented Dec 1, 2018

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

@bmeurer

This comment has been minimized.

Copy link
Contributor

bmeurer commented Dec 3, 2018

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

This comment has been minimized.

Copy link
Contributor

trueadm commented Dec 3, 2018

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

@bmeurer

This comment has been minimized.

Copy link
Contributor

bmeurer commented Dec 3, 2018

We have a simple repro for this problem and filed v8:8538 to track this.

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Dec 3, 2018

@bmeurer Thanks for tracking this internally so quickly. Out of curiosity, do you know if the same issue exists when using Object.freeze too, or is it only with Object.preventExtensions?

@bmeurer

This comment has been minimized.

Copy link
Contributor

bmeurer commented Dec 3, 2018

Object.freeze() is fine, but only because the object is frozen 😁

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Dec 3, 2018

Thanks so much for the info @bmeurer!

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Dec 3, 2018

Initializing the values to Number.MIN_VALUE or Number.NaN is fine, but at some point those fields need to hold Smi values like 0 or our profiling tools get a lot more complicated.

My testing seems to suggest that it's sufficient to initialize the fields to double values as you've described, and then immediately replace those values with Smis. So we'll roll with that for now.

@ValentinH

This comment has been minimized.

Copy link

ValentinH commented Dec 3, 2018

Do you have an estimated time for this to be released?

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Dec 3, 2018

Not currently. I'll bring it up with the team today and see if we can't prioritize one soon though.

@bmeurer

This comment has been minimized.

Copy link
Contributor

bmeurer commented Dec 3, 2018

@bvaughn They can hold integers, no problem. It's just that they should start with doubles, otherwise the instances need to transition from Smi fields to Double fields eventually, which triggers the bug. As long as they start out as Doubles, no migration is needed.

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Dec 3, 2018

Thanks for confirming! 😄

@ValentinH

This comment has been minimized.

Copy link

ValentinH commented Dec 15, 2018

Hey @bvaughn do you know now if this will be released soon? I don't want to be annoying, but we are stuck on 16.4 because of this. 😆

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Dec 15, 2018

Don't know, no. It's been released in the last several canary tags but I don't know when our next patch release will be.

@ValentinH

This comment has been minimized.

Copy link

ValentinH commented Jan 11, 2019

@bvaughn I've tried again to update from 16.4.1 to 16.7.0 and I still have some leaks / high memory issues. 😱

Here's a capture of my task manager:
react-leak

We have several apps in my company and only one faces this issue... How could I try a development version of the devtools to investigate better?

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Jan 11, 2019

This issue was about fibers being slow for property lookup– so apps were slower, but I don't think it necessarily had any impact on memory usage. (So I think what you're reporting is probably unrelated?)

If you can share your source code, you might want to file a new issue with some info:

  • Instructions for how to checkout and run your app
  • Which version(s) or React are impacted
  • Which browsers/versions are impacted
  • Do you only see this behavior when DevTools are enabled?
    etc
@ValentinH

This comment has been minimized.

Copy link

ValentinH commented Jan 11, 2019

I managed to isolate the component that is responsible for the issue. It uses setInterval() and creates new Date() at each interval so it doesn't smell good.

I'll try to setup a reproduction example 🙂

@ValentinH

This comment has been minimized.

Copy link

ValentinH commented Jan 11, 2019

Issue created: #14574

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.