[Performance] All rows are re-rendered in the actual DOM whenever the application state changes

[ohana54]

Hi,

I discovered a major performance issue when I tried to use the grid in my application with about 1000 rows (max results set to 10, infinite scrolling enabled).
The rows are being re-rendered (in the actual DOM) whenever any state in the application changes.
This is due to a new unique key that's being generated every time a row is rendered, so react thinks it's a new row every time and replaces everything in the actual DOM.

A reproduction is shown here: http://jsfiddle.net/u0ue24ea/
Open the console and click on the "inc" button. I print to the console the DOM changes and you can see that all the rows are being replaced.

I believe this should be addressed ASAP if people are going to rely on this grid for their apps.

Thanks!

[FoxxMD]

I think this also prevents further customization of #75. When I render a custom row and then change a property of its state the row is immediately re-rendered with initial data, preventing any kind of internal interaction with the custom components.

[joellanciaux]

@ohana54 Thanks for putting together an issue. We added only rendering relevant rows infinite scrolling in master a little bit ago, but it hasn't made it's way to npm just yet. We're hoping to get this out with some other features/improvements in the near future, but you might be able to get moving with the source - I'll write up a little more on this later tonight.

This initially came about because we were a little reluctant to specify a row height when infinite scrolling, but I think that's the only way to make this happen.

[scvnc]

I have a hunch that the issue is usage of _.uniqueId for keying of components internally. I had similar problems with a different react grid doing the same thing. I was able to fix this when I removed usage of _.uniqueId in favor of requiring the user to define which property on the row object can uniquely identify a row. Otherwise every row was looking like a entirely different row since the keys get incremented every time react calls render()

After that it was able to properly diff and manipulate the DOM more efficiently. Using _.uniqueId may be a React anti-pattern.

[ohana54]

@joellanciaux I may have not been clear, the issue is present regardless of infinite scrolling. My example shows a basic grid without any fancy options. All the rows are re-rendered when any not grid related state changes. Again, I show it in the example.

@scvnc Yes, that is the root cause. I fixed it as well in the same way you did, but still experienced slow rendering (~250ms), which sent me looking for alternatives.

[scvnc]

I think PureRenderMixin might be the next piece for performance improvements. Now it's not doing all that unnecessary DOM manipulation right? So that time is probably spent recalculating the entire virtual DOM of the grid?

[joellanciaux]

@ohana54 Gotcha - @blkmutt put together something that takes care of a good bit, but I'll try looking into what's keeping things a little sluggish on large datasets (as you're running into). There's likely some 'shouldComponentUpdate' logic that needs to be added, or, as @scvnc mentioned, see how PureRenderMixin might fit into things.

@scvnc That's a good idea -- that might be a worthwhile addition. I'll be giving that a go a bit later (unless someone else beats me to it).

Semi-related, I do need to get some of these changes consolidated/documented a little bit more now that we have a few things that might belong in rowProperties (row height, etc.).

[FoxxMD]

Any update?

[anshumanf]

Fork of the JSFiddle provided by @ohana54 for anyone in an environment where the fb.me domain is blocked:

https://jsfiddle.net/anshumanf/3yem8mq1/

[conorhastings]

using unique ids as keys is considered an anti pattern ==> https://facebook.github.io/react/docs/reconciliation.html#trade-offs , particularly point 2.

[r1g0]

I had this same problem and since my table was simple I could manage with shouldUpdateComponent
https://facebook.github.io/react/docs/advanced-performance.html
Perhaps it works for someone else

[scvnc]

Even with the uniqueId fix back in May: I suspect @r1g0's solution of using your own shouldComponentUpdate is the next step. And if that isn't adequate then the grid should become immutable-data aware so you can shove down what is changing in a detectable way.

I've been out of the griddle loop for a bit but last I heard the rewrite was going to be aware of immutable data.

[scvnc]

The key was added in pull reqest #105 that launched from this thread back in May. We should probably document what happened in this issue thread someplace.

So having the key should help a bit because React will be able to know that it doesn't have to remove/insert all the dom nodes for the rows.. However, I don't think this addresses the unneeded virtual DOM recomputation time I suspect that this was the ~250ms @ohana54 was seeing earlier this year.

Taking inspiration from @r1g0, we can potentially prevent the recomputation of the virtual DOM by wrapping the grid with a shouldComponentUpdate() with some logic determining whether to make the grid re-render.

Here's an untested example that depicts this strategy

class GridWrapper extends React.Component {

  shouldComponentUpdate (nextProps) {
    // Somehow based on the 'results' prop determine if the grid should re-render

    // Do not update if it looks as if the grid results did not change.
    if(nextProps.results === this.props.results) {
      return false;
    }

    // Otherwise we should probably re-render the virtual dom
    // because we can declare something ought to update in the grid
    return true;
  }

  render () {
    return <Griddle results={this.props.results} />
  }
}

Edit: in response to a later-deleted comment :3

[jgwconsulting]

Sorry! I deleted my comment after I re-read the whole thing and realised I was pretty much just going over old ground :/

I'm using Redux and react-pure-render (https://github.com/gaearon/react-pure-render), which pretty much implements what you're suggesting, and that, in combination with specifying the key, must be why I'm seeing good (re)render times, as you say.

Just by the by, I'm loving the infinite scrolling virtualisation, it's very clever.

[scvnc]

No prob @jgwconsulting. I think this merits some more documentation based on the findings in this thread because it's a popular issue. It is valuable to know that you are getting some good performance when baking in both a key and a componentShouldUpdate strategy.