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

Performance regression for removing rows from large table #7227

Closed
krausest opened this issue Jul 8, 2016 · 12 comments
Closed

Performance regression for removing rows from large table #7227

krausest opened this issue Jul 8, 2016 · 12 comments

Comments

@krausest
Copy link

krausest commented Jul 8, 2016

In my javascript frameworks benchmark I ran into a performance regression for react.
To reproduce open the react.js 15.2 version and open the console.
Repeat three times the following: Click "Create 10,000" then click "Clear"
You'll get a result like that:
runLots took 1905.0000000000005
clear took 1997.0249999999996
runLots took 1813.8500000000004
clear took 4208.895
runLots took 1806.864999999998
clear took 401.119999999999

Now repeat the same for react.js 0.14.8 and you'll get something like that:
unLots took 2938.8900000000003
clear took 399.2250000000013
runLots took 2726.365
clear took 383.45500000000175
runLots took 2872.625
clear took 381.7250000000058

(Measurement on Chrome 51 on MacOS 10.11.5 on a MBP 15)

Notice that clear is a lot slower for the first two runs for React v 15, but the third clear run performs about the same for both react versions.

The source code can be found in the repository of my benchmark.

@gaearon
Copy link
Collaborator

gaearon commented Jul 8, 2016

In general we don’t advise you to render 10K items. Use something like https://bvaughn.github.io/react-virtualized/ instead.

However, this indeed seems like a perf regression. It appears that we’re spending a little time in willDeleteListener per each item, and it adds up with 10K items. (Scratch that, doesn’t appear to be the cause.)

@gaearon
Copy link
Collaborator

gaearon commented Jul 8, 2016

This is a very simple scenario: we are unmounting 10,000 components.
The bottleneck is somewhere in reconciliation.

React 0.14 is reconciling fast right away:

screen shot 2016-07-08 at 22 06 01

React 15 is reconciling slow at first (notice how long reconcilerUpdateChildren is compared to applying DOM updates):

screen shot 2016-07-08 at 22 03 56

But it warms up exactly after two mount-unmount cycles (notice how its size is less relative to DOM updates):

screen shot 2016-07-08 at 22 04 07

@gaearon
Copy link
Collaborator

gaearon commented Jul 8, 2016

It looks like there is some sort of V8 optimization that doesn’t kick in until you unmount a lot of components. I was able to drastically reduce time (from 2 seconds to 300 milliseconds) on React 15 by replacing delete statements with null assignment in these two places:

This is probably not a real solution (I think just leaving them as null means memory keeps growing, even if just for property keys).

Still, I wonder why 0.14.x is not similarly penalized.

@gaearon
Copy link
Collaborator

gaearon commented Jul 8, 2016

Replacing objects with ES6 Maps gives similar performance improvements there.

gaearon added a commit that referenced this issue Jul 9, 2016
As reported in #7227, unmounting performance regressed with React 15.
It seems that `delete` has become much slower since we started using numeric keys.
Forcing dictionary keys to start with a dot fixes the issue.
@gaearon
Copy link
Collaborator

gaearon commented Jul 9, 2016

This was fixed in #7232. Thank you very much for caring and providing specific instructions.

@gaearon gaearon closed this as completed Jul 9, 2016
@julienw
Copy link

julienw commented Jul 9, 2016

As a quick experiment, I ran the benchmarks under Firefox: both versions ran in the same speed as the 15.2 in Chrome (I mean: slow). I surely hope the fix would fix it in Firefox too !

@gaearon
Copy link
Collaborator

gaearon commented Jul 9, 2016

Maybe putting these fields on the internal instances instead of global maps will help it in Firefox. Would you like to investigate why it's slow?

@gaearon
Copy link
Collaborator

gaearon commented Jul 9, 2016

I gave it a look, and Firefox slowness is due to DOM removeChild being slow. I don’t think we can fix it.

@bzbarsky
Copy link

I gave it a look, and Firefox slowness is due to DOM removeChild being slow.

I'd really appreciate a Firefox bug report with testcase here!

@julienw
Copy link

julienw commented Jul 11, 2016

@bzbarsky I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1285874 with the information of this bug. I don't have the time to do a reduced testcase now though.

@smaug----
Copy link

FWIW, removeChild is slow in Gecko because of layout. No idea whether setting display: none before removal or some such might speed up the process. It might help if done on the element on which removeChild is being called.

@bzbarsky
Copy link

@julienw Thank you for filing!

zpao pushed a commit that referenced this issue Jul 13, 2016
As reported in #7227, unmounting performance regressed with React 15.
It seems that `delete` has become much slower since we started using numeric keys.
Forcing dictionary keys to start with a dot fixes the issue.
(cherry picked from commit 64e7602)
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

5 participants