Repository navigation
Conversation
- There is no real benefit in creating the wrapper function - Before the change, the Grid component holds logic related only to CellSizeAndPositionManager - Having the actual defined cell size is needed for better calculating the offset
Holding a cache of cell sizes can have a memory hit in case of very large lists, especially if the user scrolls to a deep page. This mitigates the issue in case the CellSize is a numeric value. In such a case, instead of calculating the size of all cells up to the wanted index and caching the results, we simply multiply the index by the height.
Use the new added numeric cell size for: - Getting the total size in a precise, independent, performance better way. - Finding nearest cell, without having to run into edge cases when cells weren't measured yet
Update existing unit tests for passing a numeric, or thunk CellSize. Cleanly duplicate some tests for testing both methods.
|
👍 need this ASAP, hope will be merged soon |
|
|
||
| _isCellSizeNumeric(): number { | ||
| return typeof this._cellSize === 'number'; | ||
| } |
There was a problem hiding this comment.
I don't feel like you would need to remove this. Is there any specific reason?
There was a problem hiding this comment.
I think it was due to flow type error
|
@omerts Also, this is optimizing the calculation of the cell sizes, but it doens't change the way caching is done, correct? For very large grids, dragging the scrollbar makes the grid crash out of memory. However, doing different scrolling events with the mouse doesn't cause this, because the cache is cleared after every scroll event. I was wondering if it is worth adding more logic to the cell and style caches to avoid this, when scrolling through millions of rows. |
|
@tomasro27 Sorry for the late response.
It only fixes the issue for fixed width/height. That's why I only called it "mitigate" the issue.
Correct, since it does all calculation with arithmetic operation there is no need for a cache.
IMHO if cache is cleared on every scroll event this is even a greater reason to add this. If you keep on scrolling down, and cache is invalidated, the component will have to recalculate all the cells up to the current cell every time. This will fix this issue for fixed size rows/columns. Also, dragging the scrollbar is a necessity if for large lists when you want to scan through them. Not having this, is kind of like having a video player that only allows you to seek with 15 seconds jumps, and not be able to seek with the mouse to the middle. |
|
Hi @omerts @tomasro27 can I ask the progress of this issue? It seems very promising... |
|
What would it take to move this forward? |
|
Can we get an update on this? I'm Dealing with 200 million rows and it would be awesome to move this forward. |
Codecov Report
@@ Coverage Diff @@
## master #1343 +/- ##
==========================================
- Coverage 90.39% 90.35% -0.04%
==========================================
Files 59 59
Lines 1738 1752 +14
==========================================
+ Hits 1571 1583 +12
- Misses 167 169 +2
Continue to review full report at Codecov.
|
|
@omerts @wuweiweiwu @tomasro27 @bvaughn Where does this stand? What needs to be done to move this forward? We experiencing some performance issues that this is going to address |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1343 +/- ##
=========================================
Coverage ? 90.35%
=========================================
Files ? 59
Lines ? 1752
Branches ? 0
=========================================
Hits ? 1583
Misses ? 169
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
Sorry for the very long delay in getting back to you about this! I've just finished a large update for version 10 of react-virtualized (#1900): the library has been rewritten in TypeScript, every component is now a function component (React 18+ is required), and the source moved from If you're still interested in this change, I'd welcome a new PR with it rebased onto the |
11 participants
Current implementation of
CellSizeAndPositionManageris prune to out of memory exceptions and laggy behavior when dealing with very large lists. Specifically this is due to the fact that many cells sizes are calculated, and stored in cache, causing a memory bloat, and slower performance. This pull request mitigates the issue in cases where the CellSize is a numeric value, so at least there is a way of working with very large lists. It does so, by arithmetically calculating offsets, size, and position. As the size is static for every cell there is no need to iterate over other cells to figure out these values.It can be easily reproduced in the demo just by raising the List's number of rows to 40 million items, and pulling the scroll down.
Before the fix (pull scroller down was also laggish):
After fix (scroller was very responsive):
