Fix performance issues for large reports#32
Conversation
|
Test QC report built! You can download it here. |
|
What to test here: changing around:
For each of these, do they persist across:
|
|
Also, as a bonus: test for #21 - related issues, like what happens when you run |
|
Okay, big changes that I think (??) solve the performance issues!
|
|
Looks like tagging a commit |
This comment was marked as resolved.
This comment was marked as resolved.
|
((Oh, sorry! I never mentioned that the performance is FANTASTIC! And there is honestly no noticeable lag when fast-scrolling. I have no comments on the performance at all, it's perfect.)) |
|
Okay, I think I've found a comfortable middle ground between no-scrollbar-ever and the awkwardness you found. Now the table always fills the available width (no scrollbar) until enough columns are selected that it would squish them to add any, at which point a horizontal scrollbar appears. |
While doing so, this refactor also lifts all table state up to the App component so it can live in one place. This massively simplifies state management, also by offering a setTableState callback which will only change the keys provided, instead of mutating the entire state. IndexedDB values are updated asynchronously, using only React state for local UI changes.
Instead of rendering all rows, use react-table's virtualization (i.e., only display ~20 rows at a time instead of all). This comes with some usability drawbacks, mainly because our rows aren't a predictable size, so there can be some flicker if scrolling fast. This seems to solve *some* of the lag issues, most importantly on first selection of a row.
This keeps state regarding datasets out of App, which prevents unnecessary re-renders when data changes
Instead of adding the invisible 'position' column, the injection of which was causing lengthy laggy re-renders, we instead make use of manual sorting. Switching it on any time a dataset is modified, we achieve freezing the rows without a re-render. When sorting changes, we flip this flag back off, and sorting resumes. This completely eliminates the awkward RowOrder state as well :) This requires moving useKeyboardShortcuts back down into Table, so we can intercept the call to change a dataset and use it to toggle sorting state. I think this leaves the components in a fairly balanced state. [skip ci]
elhil
changed the title
joshuacwnewton
left a comment
joshuacwnewton
left a comment
There was a problem hiding this comment.
Everything works beautifully!
The only issue I noticed (outside the scope of this PR) is that refresh_qc_entries.py wipes out rank/QC. This is... quite severe! But, I don't know if anyone actually uses that feature (I found it hard to productively delete specific json files, since the filename don't include any subject/session details), so hopefully (🙏) no one has run into this before.
I wish I could review the actual code changes for like... TypeScript code quality, structure, maintainability, etc. but I don't have any experience in terms of like... TS patterns, whatever the equivalent to "Pythonic" is for webdev. Still, everything works like a charm, so... good to go? 😅
Oh, it shouldn't. That should work outside this PR, so that would be a regression. I'll look into it!
Happy to run through the codebase at any point, though the only way to really judge this would be to learn how to do React development, which I don't necessarily recommend 😅 |
|
Okay, I think the QC/rank wipeout is an upstream issue, tracked in spinalcordtoolbox/spinalcordtoolbox#5177 and spinalcordtoolbox/spinalcordtoolbox#5178. (Did this ever work?) |
elhil
force-pushed
the
indexeddb-storage
branch
from
9d2005d to
ecea64d
Compare
|
(Force-pushed rebased version for one final test) |
|
Test QC report built! You can download it here. |
Works great using index.html from this build in the example report from upstream. Using Merging! Thanks @joshuacwnewton as always for the careful testing and detailed feedback. |
2 participants
It turns out that browsers set a fairly small quota per website for localstorage. Since all our reports share the same localstorage pool, browsers were running out of space, causing spinalcordtoolbox/spinalcordtoolbox#5083
Hence, implements IndexedDB as our storage backend: asynchronous, fast, and allowing ~10GiB on most browsers.
In addition, applies several optimizations:
This also cleans up the code a little by consolidating state management, which needed to happen eventually anyway.