Skip to content

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

Closed
@bvaughn

Description

@bvaughn
Contributor

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.)

Activity

changed the title [-]Possible v8 de-opt [WIP][/-] [+]Possible v8 de-opt for profiling react-dom bundles[/+] on Nov 30, 2018
bvaughn

bvaughn commented on Nov 30, 2018

@bvaughn
ContributorAuthor
mathiasbynens

mathiasbynens commented on Nov 30, 2018

@mathiasbynens

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

bvaughn

bvaughn commented on Nov 30, 2018

@bvaughn
ContributorAuthor

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

mathiasbynens commented on Nov 30, 2018

@mathiasbynens

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

bvaughn

bvaughn commented on Nov 30, 2018

@bvaughn
ContributorAuthor

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

trueadm commented on Dec 1, 2018

@trueadm
Contributor

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

mathiasbynens commented on Dec 1, 2018

@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

uriklar commented on Dec 1, 2018

@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

mathiasbynens commented on Dec 1, 2018

@mathiasbynens

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

uriklar

uriklar commented on Dec 1, 2018

@uriklar

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

bvaughn

bvaughn commented on Dec 1, 2018

@bvaughn
ContributorAuthor

Thanks @uriklar!

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

mathiasbynens

mathiasbynens commented on Dec 1, 2018

@mathiasbynens

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

bmeurer

bmeurer commented on Dec 3, 2018

@bmeurer
Contributor

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

trueadm commented on Dec 3, 2018

@trueadm
Contributor

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

9 remaining items

bmeurer

bmeurer commented on Dec 3, 2018

@bmeurer
Contributor

@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

bvaughn commented on Dec 3, 2018

@bvaughn
ContributorAuthor

Thanks for confirming! 😄

ValentinH

ValentinH commented on Dec 15, 2018

@ValentinH

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

bvaughn commented on Dec 15, 2018

@bvaughn
ContributorAuthor

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

ValentinH commented on Jan 11, 2019

@ValentinH

@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

bvaughn commented on Jan 11, 2019

@bvaughn
ContributorAuthor

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

ValentinH commented on Jan 11, 2019

@ValentinH

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

ValentinH commented on Jan 11, 2019

@ValentinH

Issue created: #14574

added a commit that references this issue on Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @bvaughn@mathiasbynens@bmeurer@trueadm@uriklar

        Issue actions

          Possible v8 de-opt for profiling react-dom bundles · Issue #14365 · facebook/react