Uh oh!
There was an error while loading. Please reload this page.
perf(start-client-core): O(1) buffer drain in client frame decoder - #8009
Conversation
The frame decoder dropped consumed chunks from its buffer with bufferList.shift(), which is O(n). When a single large frame (e.g. a big RawStream payload) is assembled from many small network reads, the extract loop calls shift() once per chunk, making reassembly O(n^2). Track the first un-consumed chunk with a head pointer and advance it in O(1) instead of shifting. Consumed slots are released for GC, and the buffer is compacted when fully drained (O(1) reset) or once the consumed prefix grows past a small threshold (amortized O(1) per chunk). A micro-benchmark draining 1000 small chunks is ~11x faster.
View your CI Pipeline Execution ↗ for commit 0e16ec4
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughThe frame decoder now uses an O(1) head pointer and batched cleanup for consumed chunks. Header parsing and payload extraction start at the active head. Tests cover highly fragmented raw payloads and JSON frames. ChangesFrame decoder performance
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🚀 Changeset Version Preview1 package(s) bumped directly, 13 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/start-client-core/src/client-rpc/frame-decoder.ts`:
- Around line 143-152: Update advanceBufferHead so bufferList.splice is
performed only when the consumed bufferHead prefix is a substantial fraction of
the remaining list, while preserving the full-drain reset behavior and
bufferHead handling. Remove the fixed 32-consumption trigger that causes
repeated large prefix relocations, and update the changeset to reflect the
restored amortized O(1) draining complexity.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13aefb37-aab4-47dc-9af2-2801c75aaf09
📒 Files selected for processing (3)
.changeset/perf-frame-decoder-index-pointer.mdpackages/start-client-core/src/client-rpc/frame-decoder.tspackages/start-client-core/tests/frame-decoder.test.ts
| function advanceBufferHead(): void { | ||
| bufferList[bufferHead++] = EMPTY_BUFFER | ||
| // Reset drained buffers immediately and compact long-lived buffers in batches. | ||
| if (bufferHead === bufferList.length) { | ||
| bufferList.length = 0 | ||
| bufferHead = 0 | ||
| } else if (bufferHead >= 32) { | ||
| bufferList.splice(0, bufferHead) | ||
| bufferHead = 0 |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Restore amortized O(1) buffer draining.
Line 151 calls splice(0, bufferHead) after every 32 consumed chunks. splice relocates every remaining chunk. A frame buffered as N one-byte chunks performs O(N / 32) prefix splices and O(N²) total relocation work during extraction.
Compact only when the consumed prefix is a substantial fraction of bufferList. This keeps the total number of relocated chunks proportional to consumed chunks. Update the changeset after the complexity claim is true.
Proposed fix
- } else if (bufferHead >= 32) {+ } else if (+ bufferHead >= 32 &&+ bufferHead * 2 >= bufferList.length+ ) {
bufferList.splice(0, bufferHead)
bufferHead = 0
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| functionadvanceBufferHead(): void{ | |
| bufferList[bufferHead++]=EMPTY_BUFFER | |
| // Reset drained buffers immediately and compact long-lived buffers in batches. | |
| if(bufferHead===bufferList.length){ | |
| bufferList.length=0 | |
| bufferHead=0 | |
| }elseif(bufferHead>=32){ | |
| bufferList.splice(0,bufferHead) | |
| bufferHead=0 | |
| functionadvanceBufferHead(): void{ | |
| bufferList[bufferHead++]=EMPTY_BUFFER | |
| // Reset drained buffers immediately and compact long-lived buffers in batches. | |
| if(bufferHead===bufferList.length){ | |
| bufferList.length=0 | |
| bufferHead=0 | |
| }elseif( | |
| bufferHead>=32&& | |
| bufferHead*2>=bufferList.length | |
| ){ | |
| bufferList.splice(0,bufferHead) | |
| bufferHead=0 |
🤖 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/start-client-core/src/client-rpc/frame-decoder.ts` around lines 143
- 152, Update advanceBufferHead so bufferList.splice is performed only when the
consumed bufferHead prefix is a substantial fraction of the remaining list,
while preserving the full-drain reset behavior and bufferHead handling. Remove
the fixed 32-consumption trigger that causes repeated large prefix relocations,
and update the changeset to reflect the restored amortized O(1) draining
complexity.
Merging this PR will degrade performance by 40.07%
|
| Mode | Benchmark | BASE | HEAD | Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | mem server error-paths redirect (solid) | 588.2 KB | 519.1 KB | +13.32% |
| ⚡ | Memory | mem client preload-churn (vue) | 776.2 KB | 737.8 KB | +5.2% |
| ⚡ | Simulation | ssr control-flow unmatched 404 (react) | 67.2 ms | 63.9 ms | +5.08% |
| 👁 | Simulation | ssr dehydrate plain control (solid) | 119.6 ms | 129.3 ms | -7.54% |
| 👁 | Simulation | ssr dehydrate rich types (solid) | 87.3 ms | 99.1 ms | -11.89% |
| 👁 | Memory | mem server error-paths redirect (vue) | 294 KB | 411.2 KB | -28.52% |
| 👁 | Memory | mem server server-fn-churn (vue) | 263.1 KB | 4,251.2 KB | -93.81% |
| 👁 | Memory | mem server error-paths unmatched (react) | 263.6 KB | 715.2 KB | -63.14% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/frame-decoder-index-pointer-7663 (0e16ec4) with main (c568caf)
Supersedes #7663. The original PR comes from a private fork, and GitHub could not update it because it conflicts with the since-merged zero-copy decoder work in #7662.
The original commit was cherry-picked onto current
main, preserving Yagiz Nizipli as author.Conflict resolution
Tests
CI=1 NX_DAEMON=false pnpm nx run @tanstack/start-client-core:test:unit --outputStyle=stream --skipRemoteCache -- tests/frame-decoder.test.ts(21 passed)CI=1 NX_DAEMON=false pnpm nx run @tanstack/start-client-core:test:types --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @tanstack/start-client-core:test:eslint --outputStyle=stream --skipRemoteCache(no errors; existing unrelated warnings)pnpm prettier --check .changeset/perf-frame-decoder-index-pointer.md packages/start-client-core/src/client-rpc/frame-decoder.ts packages/start-client-core/tests/frame-decoder.test.tsSummary by CodeRabbit
Performance
Bug Fixes