perf(virtual-core): drop closures from calculateRange on the lanes===1 hot path#1206
perf(virtual-core): drop closures from calculateRange on the lanes===1 hot path#1206mds-ant wants to merge 3 commits into
calculateRange on the lanes===1 hot path#1206Conversation
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/virtual-core/tests/bench.bench.tsParsing error: "parserOptions.project" has been provided for 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
1 participant
🎯 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 by CodeRabbit
Bug Fixes
Tests