FormidableLabs / radium

A toolchain for React component styling.
http://formidable.com/open-source/radium/
MIT License
7.39k stars 308 forks source link

Radium seems to slow down framerates #389

Open Aaike opened 8 years ago

Aaike commented 8 years ago

Hi , i have made a simple test with react motion.

As soon as i add the @Radium decorator it slows down quite a lot. The difference isn't really noticable on desktop, but on a mobile device it is a big difference.

please see the tests here : https://github.com/Aaike/react-motion-radium-test There are links to 2 tests. The only difference in the 2 demos is the addition of the @Radium decorator.

Please look at those tests with a chrome browser on a mobile device.

Do you have any suggestions to avoid this drop in performance ?

tlvenn commented 8 years ago

I experience the exact same thing, tested on Nexus 5 using chrome remote debugging and port forwarding.

tlvenn commented 8 years ago

@ianobermiller any idea of what could be happening ?

tlvenn commented 8 years ago

FYI, I just checked and the same thing is happening with Radium v0.14.2

Does anyone else using Radium on mobile app ? Because the issue should be obvious pretty fast if you do...

ianobermiller commented 8 years ago

Try adding shouldComponentUpdate to Item. PureRenderMixin would probably work fine. We knew Radium's approach to walking and potentially cloning rendered elements could be slow, and it looks like are running into it here when you have dozens of items trying to render at 60fps. But, since the props to Item itself isn't changing, you shouldn't actually need to render it again.

Of course this doesn't work if your animated props are passed as styles that you want to pass to Radium, but I'll bet in most cases you can encapsulate the Radium component on its own, with shouldComponentUpdate, and avoid the perf hit.

Also, an entire repo for a bug repro? Way to go!

ianobermiller commented 8 years ago

So, I cloned the repo and looked into this more. Turns out that shouldComponentUpdate will probably help, though I didn't test on mobile, I did some profiling. Naive sCU with the shallowCompare addon is a bit slow, since it loops through both props and state. In this case, we know the props never change (assuming we fix onChange to not be a bound function, thus changing every time), so we can just use:

  shouldComponentUpdate(nextProps, nextState) {
    return this.state !== nextState;
  }

Profiling after adding sCU showed that almost no time was spend in Radium anymore, proportionally which makes sense, since Radium is only called on render.

Since the animations are smooth, searching for a single letter, "a", causes the list to rerender 80 times within a second or so. Multiplied by 20 items, you get 1600 rerenders; it is no wonder that was slow on mobile. While testing on desktop (Chrome, recent MBP), with the proper sCU I never saw below 60fps. With worst case, sCU returning true, it would drop below 30. I'll send a PR if you can check on mobile.

Bottom line is that Radium does add a usually negligible cost to render, but if you aren't careful and render a lot, you can still run into trouble. PR forthcoming.

ianobermiller commented 8 years ago

I think we may need some documentation about this.

arush commented 8 years ago

Would love to see some docs on how and when to avoid perf issues with Radium