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

Airbnb sees ~1-2s TTI regression going from React 16.2 -> 16.5 #16106

Closed
etripier opened this issue Jul 10, 2019 · 5 comments
Closed

Airbnb sees ~1-2s TTI regression going from React 16.2 -> 16.5 #16106

etripier opened this issue Jul 10, 2019 · 5 comments

Comments

@etripier
Copy link

etripier commented Jul 10, 2019

Do you want to request a feature or report a bug?
Bug.

What is the current behavior?
We have one of the larger production React apps and recently we upgraded our version of React from 16.2.0 to 16.8.6. Immediately after the upgrade, with no other changes except bumping our react and react-dom packages, we saw a 1-2s TTI increase on many of our pages for users browsing with Chrome and Firefox (specifically, not Safari). This represents a ~15-50% increase in TTI for a few of our pages.

It's difficult to tell exactly what the issue is given the size and complexity of our app. We noticed that React spends a lot of time instantiating FiberNode classes, but haven't really found anything that stands out. We also noticed the TTI increase going from 16.2 to 16.5. 16.3 does not increase our TTI.

For some additional context: The TTI metric we use is a custom metric, defined per page. The default definition is wait until mount of the root container, fire a 0ms setTimeout, and mark when that resolves. Some pages wait until a certain API response arrives or an async component mounts, then fire a 0ms setTimeout and mark when that resolves.

We'll keep updating this thread as we collect more information, but wanted to see if anyone had run into a similar issue.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:
We can't reproduce this issue outside of the context of our app for now, and we don't want to copy/paste our source code here.

What is the expected behavior?
We experience no regression in performance on React 16.8.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
This issue appears on React >=16.5 (potentially 16.4, but we haven't tried that yet).

@etripier etripier changed the title Airbnb sees ~1s TTI regression going from React 16.2 -> 16.5 Airbnb sees ~1-2s TTI regression going from React 16.2 -> 16.5 Jul 10, 2019
@trueadm
Copy link
Contributor

trueadm commented Jul 11, 2019

Thank you for reporting this to us. It would be great if you could validate 16.4 too. That will help us narrow down the possible reasons for the regression.

@gaearon
Copy link
Collaborator

gaearon commented Jul 11, 2019

Last time we saw a report like this, it was a minifier bug (not in React but in an app itself) that caused the FiberNode constructor to get inlined at its new callsite. Something like return new (function() { ... })(). See #13987 (comment).

I would suggest to look at the minifier output first and check whether it messed with any hot paths like this. Also maybe you could try switching minifiers to check if that’s the cause (which one are you using?)

It could still be a React thing for sure though. In that case we’d need more details. Such as maybe you could try bisecting it to a specific commit.

@gaearon
Copy link
Collaborator

gaearon commented Jul 11, 2019

Another thing that’s technically possible is that maybe some bugfix affected you negatively. Such as #12586 or #12708. I wouldn’t expect that but maybe you can try reverting individual changes to see which one bit you.

@etripier
Copy link
Author

Hey Dan and Dominic! Sorry for the late response. The issue was actually exactly as described in #13987 (comment). The interesting thing is that we still noticed the problem with an older version of uglify-js (and not uglify-es). We ended up using the reduce_funcs: false option.

I saw that you're now using Closure Compiler to transpile/minify React. Would you recommend it?

@gaearon
Copy link
Collaborator

gaearon commented Jul 18, 2019

I’d recommend it for a library. I don’t have a good sense for how it works with application code although I presume that should be good too!

The only downside I’ve ran into so far is that its output can be a bit unusual and can trigger edge cases and bugs in minifiers down the chain.

I take it that we can close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants