Optimizing column/row cache update mechanism. #676
Conversation
…provement when CellMeasurer is used
| @@ -225,18 +225,29 @@ export default class CellMeasurerCache { | |||
| // :columnWidth and :rowHeight are derived based on all cells in a column/row. | |||
| // Pre-cache these derived values for faster lookup later. | |||
| // Reads are expected to occur more frequently than writes in this case. | |||
| const columnKey = this._keyMapper(0, columnIndex) | |||
| const rowKey = this._keyMapper(rowIndex, 0) | |||
| let columnWidth = 0 | |||
There was a problem hiding this comment.
There is precomputed value from constructor for minWidth.
let columnWidth = this._minWidth;| } else { | ||
| for (let i = 0; i < this._rowCount; i++) { | ||
| columnWidth = Math.max(columnWidth, this.getWidth(i, columnIndex)) | ||
| } | ||
| } | ||
| let rowHeight = 0 |
There was a problem hiding this comment.
There is precomputed value from constructor for minHeight.
let rowHeight = this._minHeight;| for (let i = 0; i < this._rowCount; i++) { | ||
| columnWidth = Math.max(columnWidth, this.getWidth(i, columnIndex)) | ||
| if (this._hasFixedWidth) { | ||
| // if has fixed width columns, no need to calculate width for each row |
There was a problem hiding this comment.
If the hasFixedWidth is true then cell measurer cache can only use defaultWidth. Or you can substitute this block with getWidth function. Look at the implementation of getWidth it is basicly doing the same thing as you in this block of code.
columnWidth = this.defaultWidth;
// or
columnWidth = this.getWidth(rowIndex, columnIndex);
There was a problem hiding this comment.
This is a reasonable point!
I believe we can skip doing any work in the fixed-width or fixed-height case. It's expected that defaultWidth or defaultHeight will be used in those cases so there isn't really any need to even set the value. 😁
| for (let i = 0; i < this._columnCount; i++) { | ||
| rowHeight = Math.max(rowHeight, this.getHeight(rowIndex, i)) | ||
| if (this._hasFixedHeight) { | ||
| // if has fixed height rows, no need to calculate height for each column |
There was a problem hiding this comment.
If the hasFixedHeight is true then cell measurer cache can only use defaultHeight. Or you can substitute this block with getHeight function. Look at the implementation of getHeight it is basically doing the same thing as you in this block of code.
rowHeight = this.defaultHeight;
// or
rowHeight = this. getHeight(rowIndex, columnIndex);
There was a problem hiding this comment.
Hey @RaviDasari.
First off, you're welcome!
Secondly- I'm excited to see a first time contributor submitting a PR to react-virtualized. Welcome! I'm also excited to see that someone else is already reviewing this PR. I wish that both of these things happened more often around here! 😁
Looks like you spotted an inefficient/unnecessary loop in CellMeasurerCache. Great work!
| for (let i = 0; i < this._rowCount; i++) { | ||
| columnWidth = Math.max(columnWidth, this.getWidth(i, columnIndex)) | ||
| if (this._hasFixedWidth) { | ||
| // if has fixed width columns, no need to calculate width for each row |
There was a problem hiding this comment.
This is a reasonable point!
I believe we can skip doing any work in the fixed-width or fixed-height case. It's expected that defaultWidth or defaultHeight will be used in those cases so there isn't really any need to even set the value. 😁
|
Thanks @RaviDasari for the quality PR and @marcelmokos for the review! Look for this to go out in a release sometime this evening. |
First of all, thanks for this great library.
The issue I faced was, when I used Table with CellMeasurer columns with high volume data (around 200 thousand records) the scroll performance is super bad :( . And this performance issue is directly proportionate to the rowCount and number of Columns with CellMeasurer. So I imagined there must be a loop involved, so I profiled and found this issue in CellMeasurerCache.
If you want to see the issue yourself, you could bump up the record count for "CELL MEASURER: mixed fixed and dynamic height text" - demo and start scrolling.
With this fix we get huge performance improvement when CellMeasurer is used.
Cheers,
Ravi Dasari