perf(virtual-core): drop closures from calculateRange on the lanes===1 hot path#1206
Merged
Merged
Conversation
Exercises `calculateRange` and the memo machinery the way a real scroll does: bump `scrollOffset`, recompute range. None of the existing benches covered the per-scroll-frame path.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR replaces the per-scroll closure-based ChangesRange Calculation Allocation Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…t path `scrollOffset` is a memo dep, so the body runs on every scroll event. Before: a 5-field options object, two `getStart`/`getEnd` closures, and polymorphic indirect calls during binary search. After: positional args; a monomorphic `findNearestBinarySearchFlat` that indexes the `Float64Array` directly.
mds-ant
force-pushed
the
perf/zero-alloc-calculate-range
branch
from
0c0cf35 to
bc4d81e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/virtual-core/tests/bench.bench.ts (1)
210-212: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winKeep benchmark offsets within realistic scroll bounds.
For
n=1000,i * 5overshoots max scroll offset for a large tail of iterations, so the loop benchmarks a clamped end-state repeatedly. Consider wrapping/clamping against the computed max offset to keep frame samples representative across sizes.♻️ Suggested tweak
bench(`n=${n}, 10k scrolls`, () => { const v = makeVirt(n) v.scrollRect = { width: 800, height: 600 } + const maxOffset = Math.max(v.getTotalSize() - v.scrollRect.height, 0) for (let i = 0; i < 10_000; i++) { - v.scrollOffset = i * 5 + v.scrollOffset = maxOffset === 0 ? 0 : (i * 5) % maxOffset ;(v as any).calculateRange() } }) bench(`n=${n}, 10k scrolls + getVirtualItems`, () => { const v = makeVirt(n) v.scrollRect = { width: 800, height: 600 } + const maxOffset = Math.max(v.getTotalSize() - v.scrollRect.height, 0) for (let i = 0; i < 10_000; i++) { - v.scrollOffset = i * 5 + v.scrollOffset = maxOffset === 0 ? 0 : (i * 5) % maxOffset v.getVirtualItems() } })Also applies to: 218-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/virtual-core/tests/bench.bench.ts` around lines 210 - 212, The benchmark loop setting v.scrollOffset = i * 5 allows offset values to exceed the realistic maximum scroll offset, causing many iterations to repeatedly benchmark a clamped end-state instead of representative scroll positions. Fix this by computing the maximum scroll offset for the component and wrapping or clamping the offset calculation (i * 5) against that max value within the loop before calling calculateRange(). Apply this same wrapping/clamping approach to all similar loops in the benchmark file where scrollOffset is being set incrementally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/virtual-core/tests/bench.bench.ts`:
- Around line 210-212: The benchmark loop setting v.scrollOffset = i * 5 allows
offset values to exceed the realistic maximum scroll offset, causing many
iterations to repeatedly benchmark a clamped end-state instead of representative
scroll positions. Fix this by computing the maximum scroll offset for the
component and wrapping or clamping the offset calculation (i * 5) against that
max value within the loop before calling calculateRange(). Apply this same
wrapping/clamping approach to all similar loops in the benchmark file where
scrollOffset is being set incrementally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9742e3e-ac59-4457-b264-0f55f8a564dc
📒 Files selected for processing (3)
.changeset/calculate-range-fewer-allocs.mdpackages/virtual-core/src/index.tspackages/virtual-core/tests/bench.bench.ts
mds-ant
force-pushed
the
perf/zero-alloc-calculate-range
branch
from
bc4d81e to
a677064
Compare
For `n=1000` (30k px content, 600 px viewport), `i * 5` reached ~50k — past the ~29.4k max scroll — so a large share of iterations benched the degenerate end-of-list range instead of a representative position. Wrap with `% maxOffset` so every iteration does real binary-search and forward-walk work.
mds-ant
force-pushed
the
perf/zero-alloc-calculate-range
branch
from
a677064 to
7b5413e
Compare
Collaborator
|
Thanks @mds-ant 👏 |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Changes
Cut per-scroll-frame allocations on the default
lanes === 1path. Range computation previously allocated an options object and two closures on every scroll event; it now does the same work allocation-free, reducing GC pressure during continuous scroll.calculateRangeImpltakes positional args instead of an options object.findNearestBinarySearchFlat(flat, high, value)— monomorphic typed-array binary search that indexesflat[i*2]directly. The closure-basedfindNearestBinarySearchis kept for thelanes > 1/ no-flatfallback.lanes === 1forward-walk readsstart + sizefrom the typed array inline.Motivation
scrollOffsetis a memo dep oncalculateRange, so its body runs on every scroll event. Onmaineach run …{measurements, outerSize, scrollOffset, lanes, flat}options object,getStart/getEndclosures, andThe codebase already has a
Float64Arrayfast path forlanes === 1(_flatMeasurements). This change reads from it directly instead of through closures.Performance
main(Hz)n=1000, 10k scrollsn=100000, 10k scrollsn=1000, 10k scrolls + getVirtualItemsn=100000, 10k scrolls + getVirtualItems✅ Checklist
pnpm run test:pr.🚀 Release Impact
This PR was assisted by Claude Code.
Summary by CodeRabbit
Summary of Changes
Bug Fixes
Performance Improvements
Tests