Fix dynamic row height offset bug #4
Conversation
|
|
||
| //Row position calculation requires that rows are calculated in order | ||
| sortedRowsToRender.forEach((rowIndex) => { | ||
| rowPositions[rowIndex] = rowPositionGetter(rowIndex); |
There was a problem hiding this comment.
Does this still suffer from the n*log(n) issue you mention here?
facebookarchive/fixed-data-table#405
Or is it only m*log(n) where m is the number of rows we render?
-- edit --
If we're just operating over the rows on screen I can't imagine m*log(n) would be very slow, but if it is we could optimize the rowPositionGetter method used for this case since I think we should be able to do it in m + log(n) time.
Rambling thoughts on that below.
Would it be possible to just run rowPositionGetter for the first row in the sorted range and then compute the rest by adding the result of getRowHeight to a sum here? That should give the performance increase I'm imagining.
We'd want to push the logic into scroll helper and have it return an array of rowPositions so it could also do the work of _updateRowHeight. Also we should be wary of off by one errors since I'm not 100% sure if row 2's position is row 1's position + row 1's height or row 1's position + row 2's height (probably the first case though).
We'll still see m*log(n) time from updating the PrefixIntervalTree when we first see row heights. It'd be great if we could batch those updates as well since the m rows being rendered probably share many of the same parent nodes in that tree.
There was a problem hiding this comment.
No nLogN performance issue, I misread the code somewhere.
If m is the visible row count, we have m log m from the sort, and m log n from rowPositionGetter. The total complexity remains unchanged, since m > n.
I agree that we could manually iterate on the first position getter, reducing it to m + logN, but I think we should do that in a follow up review. I'm not imagining the priority for that is super high, plus its out of scope for this bug.
There was a problem hiding this comment.
sgtm, it's not worth the added complexity unless we see a perf issue.
|
lgtm once a plan for the perf concern is in place |
Description
Row offset calculation is dependent on a subset of previous row offsets. ScrollHelper's scrollTo or scrollBy functions calculate them correct. But you may update scrollPosition without triggering a scrollTo/scrollBy.
A simple way to test this is to scroll down a bunch, then scroll up to the first row and collapse it. See attached video.
This fix forces the sorts the rows before calculating their offsets.
This shouldn't hamper performance because visible rows should be small. It may be possible to speed this up using something like radix sort.
Motivation and Context
facebookarchive/fixed-data-table#405
How Has This Been Tested?
Tested locally but running dev site.
See video
for original repo steps.
Types of changes
Checklist: