facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.52k stars 46.76k forks source link

Performance regression for removing rows from large table #7227

Closed krausest closed 8 years ago

krausest commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Replacing objects with ES6 Maps gives similar performance improvements there.

gaearon commented 8 years ago

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

julienw commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

bzbarsky commented 8 years ago

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 commented 8 years ago

@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---- commented 8 years ago

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 commented 8 years ago

@julienw Thank you for filing!