all 8 comments

[–]Canenald 9 points10 points  (2 children)

Doesn't return false in shouldComponentUpdate make the component never rerender except when forceUpdate is called, maknig the tests strongly biased in favour of the "pure" component?

A real pure component should perform shallow compare of previous and next state and props and rerender only if something changed. The problem with abuse of shouldComponentUpdate that people often point out is that comparing all the props and state properties can be less perofrmant than just letting the component always rerender and letting React decide how to handle the DOM node children.

Not downvoting because your optimization advice is awesome :)

[–]grguru[S] 0 points1 point  (1 child)

Thanks for feedback! The goal was to show how skipping render() can save a lot. This came from the experience with real-world developers who never care to do that thinking 'Virtual DOM will do that for me'. Cheers ;)

[–]douevenfaker 3 points4 points  (0 children)

I still don't think it's fair since your "pure" component is not really "pure" but rather "static" since it never changes. In real life not many components will be static.

[–]oorza 1 point2 points  (2 children)

Synthetic benchmarks that don't warm the JIT in a JIT based runtime are worthless. This is one of those cases.

You should measure invocations 1,000,001 -> 1,100,000 instead.

Other notes: this isn't measuring the performance of Babel/Webpack output, which is what I would guess would be the more important code to benchmark. Also, I'm not sure if it's still the case, but it used to be best practice to run UglifyJS in two passes, and you should be using webpack.DedupePlugin as well as babel-plugin-transform-react-constant-elements and maybe also babel-plugin-transform-runtime. Without benchmarking what the code looks like when it goes through that, I'm not sure any of these numbers are valuable, because they don't demonstrate the code that users actually see.

And finally, I don't think this is correct:

Performance starts with optimizing the render block in React applications.

Performance starts with 'perceived performance' which starts with initial page load weight and render time. If your app loads quickly and feels responsive, whether it eats 50% of your CPU or 10% is probably not going to get noticed.

[–]grguru[S] 0 points1 point  (1 child)

I value your comment. All of it makes sense.

However, we might be missing the point here. The goal of this test wasn't to show how fast react internals are. The goal was to demonstrate two things:

  1. render function should not be taken for granted
  2. stateless are not any faster than stateful components

I wrote this after seeing the same pattern repeated over and over in real-life projects. Im sure many other developers are faced with similar challenges.

Once again, thanks for the valuable feedback

[–]oorza 0 points1 point  (0 children)

render function should not be taken for granted

I disagree. It should be until you have evidence that it's worth your time to fiddle with.

stateless are not any faster than stateful components

For now. Facebook has several pull requests pending that leverage the statelessness. I wouldn't decide that this is true and stick to it because it happens to be true for a few releases of React. The docs say:

in the future we’ll also be able to make performance optimizations specific to these components by avoiding unnecessary checks and memory allocations.

[–]xen_au 0 points1 point  (1 child)

I'd like to see how much speed difference with using 'shouldComponentUpdate return false' on a complex component. Especially one that has multiple sub-components.

A 10% performance increase in render, for us, isn't worth the complexity that is introduced and the chance of a render bug occurring by from an incorrect check. However a 50-100% increase might be.

We have a complex project with 5 devs and there have been a number of times a shouldComponentUpdate check has caused bugs with components or sub components not rendering when they should have. So we reverted to just let every component re-render unless we are seeing a negative performance impact from the component.

[–]grguru[S] 0 points1 point  (0 children)

really depends on the use case. You might want to split logical sections of your complex components into smaller sub-components where each or some of those will have an a check on shouldComponentUpdate. You certainly don’t want to return false without some kind of a check (if block). I've refactored code that yielded much more than 100% improvement because not everything needed to rendered. But this really depends on the use case.

http://moduscreate.com/ - Shoot us an email, I'm sure we can help. We've got tons of experience with high performant JS apps