[table] Perf improvements with selections

[mcintyret]

Improvements to table performance, particularly when in controlled mode (wrt selections).

Selection changes passed through props trigger componentWillReceiveProps(). Previously this would always result in new columnWidths and rowHeights set on state, even when there were no material changes, which would mean shouldComponentUpdate() always returned true, and we had a big rerender. This should be fixed now.

[mcintyret]

I don't understand the compile errors here :-(

[giladgray]

@mcintyret can you share some data on performance improvements as a result of this change?

[giladgray]

if i'm not mistaken, this will be true every time as children is a new object reference every time (never ===)?
perhaps add a console.log(hasNewChildren) locally to debug if it's ever false?

[mcintyret]

children may be memoized

[cmslewis]

nit: hasNewChildren sort of implies to me that there are additional children. maybe call this didChildrenChange.

Also, RE: "may be": do you mean the children will be memoized only if a developer does so explicitly?

[mcintyret]

Yes exactly - it needs to be explicitly done by the developer. Memoizing props is a standard part of getting better perf in react so seems reasonable?

[giladgray]

this should be optional. i think that's the source of compiler errors.

but also you have lint failures.

[mcintyret]

thanks

[giladgray]

this is a concerning deletion. input from @themadcreator @cmslewis on the impact here?

[mcintyret]

Hmm I suppose it is inconsistent with my comment above where I claim that children can be memoized, so the shallow compare would be useful. Might be better to revert to the old code and add a comment making clear that the shallow compare will fail on children unless they are memoized.

[giladgray]

columnWidths !== this.props.columnWidths is problematic as array is likely to be a new instance?

[mcintyret]

I guess my assumption here is that clients who are performance-sensitive will take care to memoize these arrays where possible, which is standard practice to get the benefits of pure-render semantics in react.

[mcintyret]

@giladgray I don't have numbers, but using the React plugin in chrome dev tools and clicking 'highlight updates' you can see that before this change selecting a cell would rerender every cell in the table, and after the change there is no unnecessary rerendering. I can look into some benchmarks tomorrow.

[giladgray]

@mcintyret neato! a GIF of the chrome tools would be sweet.

[mcintyret]

Reverted the changes to cell.tsx because the shallow comparison of children does work when children are primitive, which is probably a common case.

[mcintyret]

Before:

After:

Notes:

1. The benefits seen here are all because of the conditional grid invalidation in Table.componentWillReceiveProps(). Table.shouldComponentUpdate() still always returns true because several props are not memoized - e.g. loadingOptions={this.getEnabledLoadingOptions()} and selectionModes={this.getEnabledSelectionModes()}. More thorough optimizations would memoize these, which would lead to no table re-renders at all. This would prevent the row and column header re-renders that are seen in the 'after' video (although it should be possible to remove them anyway). The fact that the grid instance doesn't get invalidated means that same instance is passed to TableBodyCells, where the shallow comparison indicates that it doesn't need to re-render.
2. There is still one re-render in the after video. This is an artifact of how RenderMode.BATCH_ON_UPDATE is implemented - via this.getNormalizedRenderMode(). Using any other RenderMode would have prevented this extra update, although the demo app seems to be broken and won't allow selection in the dropdown.
3. To show the re-renders in the dev-tools React plugin I had to wrap the random strings in the <Cell/> elements in a <span/>. This forces the shallow comparison of children in Cell.shouldComponentUpdate() to fail, so Cell always re-renders. This doesn't mean that the efficiency gains here only apply when children are non-primitive - the 'before' version also includes calling mutableTable.renderCell() and Cell.shouldComponentUpdate() once for each cell regardless of whether the cell ends up re-rendering, while this is all skipped in the 'after' version.

[mcintyret]

Hmm now I'm having second thoughts about this. The absence of renders means that if you want to render a cell differently depending on whether it's selected or not, you can't now. But I do think having this efficiency would be useful in many cases.

Maybe have a forceRerenderOnSelectionChange flag that defaults to false to accommodate this use case?

[cmslewis]

Hi @mcintyret, I'm good with the change in spirit - just want to add some unit tests (1) to make sure there are no obvious oversights, and (2) to show how this patch of code handles various update cases. Can you write a few in tableTests.tsx?

[cmslewis]

nit: hasNewChildren sort of implies to me that there are additional children. maybe call this didChildrenChange.

Also, RE: "may be": do you mean the children will be memoized only if a developer does so explicitly?

[cmslewis]

// @tmcintyre Wowww, I wrote this comment months ago and never clicked "Submit review." Sorry about that!

[cmslewis]

@themadcreator @giladgray @adidahiya - what do you all think about this?

The table's data-agnosticism leads to an inherent tradeoff: since the data is not passed in explicitly via props, changing the data returned from a callback for an index currently in view will not lead to a re-render, so data could get stale. To solve for this, the table aggressively invalidates the current view and re-renders all cells in the viewport whenever any other prop changes - e.g. selectedRegions.

This prop keeps that behavior by default, but also allows users to disable it if they know their data won't change, leading to better performance overall.

p.s. @mcintyret all the table boolean props just moved to an enable____ naming scheme. So if we move forward with this, what's a good name?

1. enableRerenderOnSelectedRegionsChange
2. enableRerenderOnSelectionChange
3. enableAggressiveRerendering
4. enablePessimisticRerendering
5. enableForcedRerenderingOnSelectionChange
6. enableForcedRerenderOnSelectionChange
   (etc.)

stale