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

1000% performance improvement for intensive react apps #6322

Closed
amized opened this issue Mar 23, 2016 · 10 comments
Closed

1000% performance improvement for intensive react apps #6322

amized opened this issue Mar 23, 2016 · 10 comments

Comments

@amized
Copy link

amized commented Mar 23, 2016

I have a real-time redux-react app that is performance critical. I have lot’s of components on the page that may update very frequently.

I noticed that some of my animations were running jerkily and general component updates were lagging when lots of actions were being dispatched in a small space of time (say 40 per second). I run a Mac 10.9.5 with the latest chrome.

I ran performance diagnostics using the react tools and then optimised my app as much as possible (mainly using shouldComponentUpdate), making sure no time was wasted in unnecessary DOM reconciliation or rendering.

But even after doing this, my app was still lagging. So I dug deeper by running a CPU profile in chrome devtools. I noticed that at the times when my animations were running jerkily or slow, the react setState() method calls were taking up to 25 ms - 30 ms on average.

That would only allow a maximum of 30 component updates a second before performance hits the wall, which in my case is an unacceptable limit. I also ran a timeline to see if it was browser painting that was slow - turns out no. Below you can see the majority of the work was being done inside scripts, not rendering. This seemed very odd to me.

screen shot 2016 screen shot 2016-03-23 at 11 42 02 pm -03-24 at 12 11 41 am

So I looked at the function stack:

screen shot 2016-03-23 at 11 51 17 pm

In this example the setState method took 24 ms purely in scripting (not rendering) and you can see a large chuck of that was spent in ReactInputSelection.hasSelectionCapabilities, the light blue method in the bottom row.

Tracing it back this was being called from the ReactReconcileTransaction.js module.

/**
 * Ensures that, when possible, the selection range (currently selected text
 * input) is not disturbed by performing the transaction.
 */
var SELECTION_RESTORATION = {

In here, supposedly React uses ReactInputSelection to restore an input selection after <input> and <textarea> components get updated. Naturally I wanted to see what would happen if I turned it off, so I took the SELECTION_RESTORATION task out of the transaction wrapper.

//var TRANSACTION_WRAPPERS = [SELECTION_RESTORATION, EVENT_SUPPRESSION, ON_DOM_READY_QUEUEING];
var TRANSACTION_WRAPPERS = [EVENT_SUPPRESSION, ON_DOM_READY_QUEUEING];

And guess what happened:

screen shot 2016-03-23 at 11 51 07 pm

The extract same setState() calls took only 1ms - 3ms! Notice the aggregated total time went from 4.91 s in the first example to only 174 ms after turning off SELECTION_RESTORATION.

In others word, a > 1000% performance increase.

For my app this was huge, and afterwards everything ran perfectly smooth.

So I have a few questions:

  • Why is the ReactInputSelection being called ferociously on almost every element I render, when I don’t even have any <input> or <textarea> elements in my components?
  • Why does the ReactInput selection restoration take so long? Is this a bug?
  • If this is a necessary cost, then can we have a way to disable selection restoration for performance critical apps that wouldn’t need to make use of that feature
@jide
Copy link

jide commented Mar 23, 2016

Wow...

@alexeyraspopov
Copy link
Contributor

@amized have you tried ReactPerf, and printWasted in particular?

@gaearon
Copy link
Collaborator

gaearon commented Mar 23, 2016

I don’t think ReactPerf would give any results here: the author already nailed it down to a very specific issue, which is selection restoration. I don’t have a good knowledge of that part of the codebase but I wonder if we can opt out of that if there was no selection in the first place, or if we can make it faster.

@quantizor
Copy link
Contributor

@alexeyraspopov That only helps for unnecessary renders, which is a piece of the puzzle... but the OP is specifically digging into React-side performance matters.

@alexeyraspopov
Copy link
Contributor

@yaycmyk by eliminating unnecessary render cycles you will decrease amount of work per frame.

@kitten
Copy link

kitten commented Mar 23, 2016

@amized Have you tried stripping your code down to a minimum, with which you're experiencing the bug? I can't really reproduce it.

The stack is mentioning "ReactInput...abilities", so I've followed SELECTION_RESTORATION to this function, that matches the name:

function hasSelectionCapabilities(elem) {
  var nodeName = elem && elem.nodeName && elem.nodeName.toLowerCase();
  return nodeName && (
    (nodeName === 'input' && elem.type === 'text') ||
    nodeName === 'textarea' ||
    elem.contentEditable === 'true'
  );
}

But I don't really see why this would be responsible (?) Is this the same function that is taking this long in your stack?

https://github.com/facebook/react/blob/f2bb01506afd85646549c23c2e0664979ffad3d4/src/renderers/dom/client/ReactInputSelection.js

Edit: Might be connected to getActiveElement ?
Edit 2: Not really making progress https://jsfiddle.net/7snp3q66/2/

@emmenko
Copy link

emmenko commented Mar 23, 2016

Interesting!

@amized couple of questions to might help debugging further the issue:

  • which version of React are you using?
  • have you tried (if possible) using different versions of React to see if you still get the same performances?
  • are your findings on production env or also dev?

@amized
Copy link
Author

amized commented Mar 24, 2016

Thanks for all the tips, I am using React 0.14.7. I've done some further investigation.

I didn’t mention what seems to be an important fact now, that a video was playing on the page together with react components updating. (This is a JW player using flash streaming)

I did some more experiments with adding/removing the video:

Test 1:
No video + SELECTION_RESTORATION
Aggregated total time of setState(): 277ms

screen shot 2016-03-24 at 2 24 52 pm

Test 2:
Video + SELECTION_RESTORATION
Aggregated total time of setState(): 1.72s

screen shot 2016-03-24 at 2 25 01 pm

Test 3:
No Video + No SELECTION_RESTORATION
Aggregated total time of setState(): 283ms

screen shot 2016-03-24 at 2 25 07 pm

Test 4:
Video + No SELECTION_RESTORATION
Aggregated total time of setState(): 266ms

screen shot 2016-03-24 at 2 25 17 pm

Given the bug only seems to be triggered with Flash, I wouldn't expect this to be on the top of the priority list to fix, although it may be indicative of some other problem in SELECTION_RESTORATION.

@jimfb
Copy link
Contributor

jimfb commented Mar 24, 2016

Your initial flame graph showed ReactInputSelection.hasSelectionCapabilities taking ~5ms, which was a red flag because if you look at the implementation of that function, you will see that all it does is read a couple of short string properties and do a couple equality checks.

I wonder if reading properties off a flash object requires doing some expensive IPC / context switches. We could potentially special case flash, but flash is dying anyway so I'm not sure it's worth the headache. We would probably take a PR for this, but probably wouldn't do it ourselves because there are higher priority issues.

@jimfb
Copy link
Contributor

jimfb commented Apr 15, 2016

I'm going to close this out, since I don't think it's something that we would invest in. We would probably take a PR for this if anyone cares to implement it.

@jimfb jimfb closed this as completed Apr 15, 2016
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

8 participants