uws: fix O(n^2) BackPressure buffer compaction and reallocation on large writes - #34023
uws: fix O(n^2) BackPressure buffer compaction and reallocation on large writes#34023robobun wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesBackpressure buffering
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:01 AM PT - Jul 14th, 2026
❌ @robobun, your commit 3eefad5 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34023That installs a local version of the PR into your bun-34023 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — CI-only test skip following the existing isCI && isLinux pattern.
What was reviewed:
- Verified
isIntelMacOSis exported fromtest/harness.ts(isMacOS && process.arch === "x64"). - Skip is gated on
isCI, so the test still runs locally and on arm64 macOS / Windows CI. - Comment documents the observed failure and why the timeout wasn't raised again.
Extended reasoning...
Overview
This PR extends an existing CI skip condition on a single slow test in test/js/node/http/node-http-backpressure-max.test.ts. The 4 GiB backpressure test was already skipped on isCI && isLinux; this adds isIntelMacOS to that same condition after the test hit its 120s timeout on the darwin-x64 runners. The diff also fixes a typo in the existing skip comment and consolidates the timeout-rationale comment into the skip comment.
Security risks
None. Test-only change, no production code touched. The skip predicate uses existing harness booleans and only narrows where the test runs on CI.
Level of scrutiny
Low. This is a mechanical CI-flake mitigation that follows an established pattern already present on the same line. The skip is scoped to isCI, so local runs and non-Intel-macOS CI lanes are unaffected. The 4 GiB drain path retains coverage on arm64 macOS and Windows, so this is not silently dropping the property being tested — the reasoning is documented inline per repo guidelines for skips.
Other factors
- Confirmed
isIntelMacOSexists intest/harness.ts:25asisMacOS && process.arch === "x64", so the import resolves. - The PR description provides concrete timing data across 16 runs showing the test sits at 75-120s on Intel macOS, which is right at the current 120s limit — the flake is well-characterized rather than speculative.
- The bug hunting system found no issues.
- No prior reviewer comments to address.
|
Build 72802 on 3eefad5: both The only red on that build is All review threads are resolved. Ready for a maintainer to merge past the unrelated main break. |
…rge writes node-http-backpressure-max.test.ts was timing out at 120s on darwin x64 CI (build 72095). A 4 GiB res.write() through node:http took 36s on a release build (75-120s on the slow Intel mac runners) vs 2.2s in Node.js. Two compounding costs in the uws BackPressure std::string, both scaling with the buffered size: - BackPressure::erase() compacted the string (front-erase memmove + shrink_to_fit realloc+copy) every time pendingRemoval exceeded 1/32 of the buffer, so draining 4 GiB did roughly 32 compactions moving ~60n bytes total. - HttpResponse::write() splits payloads at INT_MAX and adds chunk framing as separate AsyncSocket::write calls; each append after the first could trigger a realloc+copy of the whole multi-GB buffer. For 4 GiB the trailing 1-byte UINT_MAX remainder alone realloc'd the 4 GiB string to 8 GiB (2.5s on its own). Fix: - erase(): compact only once half the buffer is dead space (O(n) total over a full drain), drop the redundant shrink_to_fit, and widen pendingRemoval to size_t. - HttpResponse::write(): reserve the backpressure buffer once up front for writes that will certainly be buffered, so the split appends never realloc. - getBufferedAmount(): report unsent bytes (length()), not totalLength(), so drain progress is observable between compactions. This is what every caller (WebSocket drain handler, maxBackpressure check, shutdown gate) actually wants. 4 GiB through node:http, linux release: 36s -> 5.3s (Node.js: 2.2s; the remaining gap is the one-time copy of the payload into the std::string which Node avoids by keeping a ref and writev'ing slices). The test timeout is lowered 120s -> 30s, which is comfortably above the measured fixed times and fails on an unfixed build.
0921426 to
5c323ef
Compare
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/js/node/http/node-http-backpressure-max.test.ts`:
- Around line 14-15: Update the skip condition for the 4 GiB backpressure test
to include Intel macOS CI by importing and applying isIntelMacOS alongside the
existing isCI and isLinux checks. Preserve execution outside CI and on
unsupported platforms.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: be2897a1-49bd-4abf-91a0-16386f29f0be
📒 Files selected for processing (4)
packages/bun-uws/src/AsyncSocket.hpackages/bun-uws/src/AsyncSocketData.hpackages/bun-uws/src/HttpResponse.htest/js/node/http/node-http-backpressure-max.test.ts
…) on totalLength() Reserving on every write once backpressure exists defeats libc++'s geometric append growth (reserve() there does not 2x), so a stream of small chunked writes to a stalled socket would realloc+copy the whole buffer on each call. Reserve only when the single write is large enough to be split; small writes keep append()'s amortised growth. WebSocket::memoryCost() feeds reportExtraMemoryAllocated and wants the allocation footprint, not unsent bytes; read totalLength() there (which also keeps that helper in use).
libc++ std::string::reserve() is exact-fit (no 2x growth), so repeated >1MB writes to a stalled socket would still realloc+copy on each call. Request max(2*capacity, need) so the reserve is amortised O(1) on any stdlib while the single-4GB case still reserves exactly once.
write() recurses into itself for payloads > UINT_MAX, so the reserve block fires again on each recursive chunk. Without the guard, each re-entry requested at least 2*capacity even when the outer call had already sized the buffer, so a 4 GiB write reserved 8 GiB and k UINT_MAX-sized chunks grew as initial*2^k.
There was a problem hiding this comment.
All three of my earlier inline findings (libc++ exact-fit reserve() on small writes, memoryCost() semantics after the getBufferedAmount() change, and recursive doubling on the UINT_MAX split) are addressed in 3eefad5, and a fresh pass on that revision found nothing new. Deferring to a human because this touches the uws backpressure buffer that every HTTP/WebSocket write goes through, and changes getBufferedAmount() semantics for all callers.
Checked this run: the BackPressure::erase threshold change (1/32 → 1/2, shrink_to_fit dropped) keeps total memmove O(n) and clear() still releases capacity on full drain; pendingRemoval widening to size_t and both erase() call-site casts; the need > capacity guard now makes recursive UINT_MAX chunks no-op the reserve. Also looked at the sibling INT_MAX-split loop in internalEnd() (res.end(largeBuffer)) — it goes through std::string::append's own geometric growth with no interleaved framing appends, so it stays O(n) without a reserve.
Extended reasoning...
Overview
Performance fix in packages/bun-uws (vendored uWebSockets) targeting O(n²) behavior when writing multi-GB responses through node:http. Touches four C++ headers on the hot path for every HTTP response and WebSocket send:
AsyncSocketData.h:BackPressure::erase()compaction threshold 1/32 → 1/2,shrink_to_fit()removed from erase,pendingRemovalwidenedunsigned int→size_t.AsyncSocket.h:getBufferedAmount()now returnsbuffer.length()(unsent bytes) instead ofbuffer.totalLength()(raw string size). Twoerase()casts widened.HttpResponse.h: new geometric-growthreserve()block ahead of the INT_MAX/UINT_MAX split inwrite().WebSocket.h:memoryCost()switched to readbuffer.totalLength()directly (restores GC-hint fidelity after thegetBufferedAmount()change).- Test timeout lowered 120s → 30s as the regression guard.
Security risks
None identified. No parsing of untrusted input, no auth/crypto/permissions. The size arithmetic (bp.totalLength() + length + 32, capacity() * 2) is on size_t and only feeds std::string::reserve; on 64-bit platforms overflow requires an ~exabyte-scale write which cannot reach this path.
Level of scrutiny
High. This is production-critical networking code — the backpressure buffer sits under every Bun.serve/node:http response and every ServerWebSocket send. The getBufferedAmount() semantic change fans out to the WebSocket drain handler, the maxBackpressure DROPPED gate, the sendStatus() reporter, and the HTTP connection-close/shutdown gates in HttpResponse.h and HttpContext.h. The reserve logic depends on libc++ vs libstdc++ growth behavior. This PR already went through three rounds of review-driven fixes on exactly those axes, which is itself a signal that a maintainer sign-off is warranted.
Other factors
- The PR description references earlier maintainer feedback that redirected this from a test-skip to a real fix, so a maintainer is already in the loop.
- The 30s timeout is a CI-runner-timing-dependent regression guard on a lane (darwin x64) that previously sat at 75-120s; a human should confirm the headroom is comfortable.
- Memory-retention trade-off: with
shrink_to_fit()gone fromerase(), a peak-sized allocation is held until the buffer fully drains andclear()runs. That's the intended trade for O(n) drain, but worth a human ack. - No new automated test for the many-small-writes-under-backpressure path that the first two review rounds were about; coverage there is reasoning-only.
…34824) ### What does this PR do? Replaces the `std::string`-backed `uWS::BackPressure` with a single malloc'd slab tracked by `head`/`tail` cursors, so draining a backpressured socket is a pointer bump instead of a front-erase `memmove` followed by a `shrink_to_fit` realloc. ### Why? The previous shape: ```cpp void erase(unsigned int n) { pendingRemoval += n; if (pendingRemoval > (buffer.length() >> 5)) { buffer.erase(0, pendingRemoval); // memmove(remaining) buffer.shrink_to_fit(); // alloc(remaining) + memcpy(remaining) + free(old) pendingRemoval = 0; } } ``` A full drain of N buffered bytes crosses that 1/32 threshold ~32 times, and each crossing moves roughly the entire remaining buffer twice (once for the front-erase memmove, once for the shrink realloc). `append()` separately went through `std::string::append`, which reallocates and copies the whole buffer (including the dead `pendingRemoval` prefix) whenever capacity is exceeded. ### Fix `BackPressure` is now `char *buf; size_t head, tail, cap;` with the data contiguous in `[head, tail)`: - `erase(n)` bumps `head`; on full drain it resets both cursors to 0 and frees. - `append()` / `resize()` write at `tail`. When the tail would overrun `cap`, first try compacting into the drained head gap (one `memmove`); only grow when the live bytes plus the new bytes genuinely do not fit. Growth uses `realloc()` when `head == 0` so mimalloc / glibc can extend in place, and drops dead head bytes otherwise. - `getBufferedAmount()` now reports `length()` (unsent bytes); `memoryCost()` reports `totalLength()` (allocation footprint) so GC extra-memory reporting keeps reflecting the real heap allocation. - `clear()` still releases the allocation, matching the previous behaviour on full drain. The API (`data()`, `length()`, `size()`, `resize()`, `reserve()`, `append()`, `erase()`, `clear()`, `totalLength()`) is unchanged and `data()` still spans `length()` contiguous bytes, so no call site other than `getBufferedAmount` / `memoryCost` had to change. ### Relationship to #34023 That PR raises the 1/32 compaction threshold to 1/2 and drops the `shrink_to_fit`, which removes the worst of the repeated realloc. This PR goes further by making `erase()` free of any data movement and letting `append()` reuse the drained head space without growing. If #34023 lands first the conflict is trivial (both rewrite the same small struct). ### Measurements Release build, `ws.sendBinary` through `Bun.serve` with a raw-socket drain (linux x64, average of 3): | pattern | main | this PR | | --- | --- | --- | | stream 256MB through a 8MB backpressure window | 223ms | 195ms | | single 256MB send then drain, peak RSS over baseline | +311MB | +311MB | The steady-state streaming case is faster because each drain no longer memmoves and reallocates the live window; the single-send peak is unchanged because both implementations hold one ~256MB buffer and the brief `shrink_to_fit` 2x spike on main is shorter than `ru_maxrss` can observe under mimalloc's mmap-backed large allocations. The new buffer retains its high-water capacity until the next full drain instead of shrinking per 1/32 step, which in the 8MB-window case shows as ~6MB higher steady RSS (56MB vs 50MB peak). ### How did you verify your code works? New integrity tests in `test/js/bun/websocket/websocket-server-backpressure-buffer.test.ts` push 32MB (direct `append` + `erase`) and 4096 x 4KB frames (cork overflow into `resize()` while repeatedly compacting) through a backpressured `ServerWebSocket` to a raw-socket client and sha1-compare every payload byte. ``` bun bd test test/js/bun/websocket/websocket-server-backpressure-buffer.test.ts (pass) BackPressure buffer > delivers a large direct send byte-for-byte while draining [2337.16ms] (pass) BackPressure buffer > delivers many corked frames while appending into a partly-drained buffer [2162.85ms] ``` Also ran `node-http-backpressure.test.ts`, `serve-response-gc-backpressure-abort.test.ts`, `serve.test.ts`, `node-http.test.ts`, `websocket-server.test.ts`; no new failures relative to main. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/websocket/websocket-server-backpressure-buffer.test.ts <!-- robobun:evidence:end -->
1 participant
What does this PR do?
Fixes two compounding costs in the uws
BackPressurestd::stringthat made largeres.write()calls throughnode:httpscale worse than linearly, and lowers thenode-http-backpressure-max.test.tstimeout from 120s to 30s now that it runs in a fraction of that.Why?
Build 72095 went red on
:darwin: 14 x64with:The test had already been bumped 60s -> 120s in #31587 for the same reason and was sitting at the new limit on the slow Intel mac runners. A 4 GiB
res.write()throughnode:httptook ~36s on a linux release build (75-120s on the darwin x64 runners) versus 2.2s in Node.js.Two problems in
packages/bun-uws:BackPressure::erase()compacted thestd::string(front-erasememmove+shrink_to_fitrealloc+copy) every timependingRemovalexceeded 1/32 of the buffer, so draining n bytes moved ~60n bytes in total.HttpResponse::write()splits payloads atINT_MAXand adds chunk framing as separateAsyncSocket::writecalls; each append after the first could realloc+copy the whole multi-GB buffer. Instrumenting a 4 GiB write:The 1-byte append is the UINT_MAX split remainder.
Fix
BackPressure::erase(): compact only once half the buffer is dead space (sum of memmove sizes over a full drain is O(n) as a geometric series), drop the redundantshrink_to_fit, and widenpendingRemovaltosize_t.HttpResponse::write(): reserve the backpressure buffer once up front for writes that will certainly be buffered, so the split appends never realloc.AsyncSocket::getBufferedAmount(): report unsent bytes (length()), nottotalLength(), so drain progress is observable between compactions. This is what every caller (WebSocket drain handler,maxBackpressurecheck, shutdown gate) actually wants; without it the less-frequent compaction stalled the WebSocket drain loop.Results (linux x64 release, raw socket client)
and for the full test (4 GiB via the fetch reader, same as CI):
The remaining ~2.4x vs Node is the one-time memcpy of the payload into the
std::string; Node keeps a reference to the caller's Buffer andwritevs slices directly. Closing that gap meansNodeHTTPResponseholding aStrongref to the JS buffer and slicing on drain instead of handing the bytes to uws's copy buffer.How did you verify your code works?
Also ran
node-http-backpressure.test.ts,node-http.test.ts,serve.test.ts,websocket-server.test.tsandserve-response-gc-backpressure-abort.test.ts; failures in those files reproduce onmainwithout this change.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/http/node-http-backpressure-max.test.ts