Closed
Description
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
[-]Possible v8 de-opt [WIP][/-][+]Possible v8 de-opt for profiling react-dom bundles[/+]bvaughn commentedon Nov 30, 2018
cc @bmeurer and @mathiasbynens
mathiasbynens commentedon Nov 30, 2018
@bvaughn Can haz repo access? I imagine @bmeurer would want access as well.
bvaughn commentedon 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 commentedon Nov 30, 2018
I mean, the more minimal the repro, the better.
bvaughn commentedon 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 commentedon 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 thatscheduler.unstable_now()
is essentially a wrapper aroundperformance.now()
and returns adouble
value, I changedscheduler.unstable_now()
so that it returned anint
(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 thefiber
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 commentedon Dec 1, 2018
Note that jsvu makes it trivial to get a
d8
binary with--prof
functionality.uriklar commentedon 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 commentedon Dec 1, 2018
It’s okay, we can wait for a reduced test case.
uriklar commentedon Dec 1, 2018
Ok i just removed the git history from Brian's repo so he can give you access now
bvaughn commentedon Dec 1, 2018
Thanks @uriklar!
@bmeurer and @mathiasbynens, you should have access now. Please let me know if anything doesn't make sense.
mathiasbynens commentedon Dec 1, 2018
Alright, we’ll take a look after the weekend :)
bmeurer commentedon 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, allFiberNode
s end up having separate shapes at some point, obviously making things likegetHostSibling()
super slow.The work-around for that on your side is to make sure to either have the time values represented as
Smi
s consistently as @trueadm suggested, or make sure to initialize them to double values from the beginning, i.e. use something liketo make sure the fields are all initialized to doubles from the beginning (as a cleaner fix I'd suggest to use
NaN
for at leastactualStartTime
, maybe all of them).trueadm commentedon Dec 3, 2018
@bmeurer @mathiasbynens A huge thanks to both of you for looking into this issue! :)
9 remaining items
bmeurer commentedon 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 commentedon Dec 3, 2018
Thanks for confirming! 😄
ValentinH commentedon 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 commentedon 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 commentedon 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:

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 commentedon 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:
etc
ValentinH commentedon Jan 11, 2019
I managed to isolate the component that is responsible for the issue. It uses
setInterval()
and creates newDate()
at each interval so it doesn't smell good.I'll try to setup a reproduction example 🙂
ValentinH commentedon Jan 11, 2019
Issue created: #14574
[Fiber] Set profiler values to doubles (#30942)
[Fiber] Set profiler values to doubles (#30942)
[Fiber] Set profiler values to doubles (#30942)