Repository navigation
Conversation
`Vec::reserve` fits a request for more than twice the capacity exactly. A byte queue that takes a large payload and then a few more bytes (a protocol trailer, a newline, a short second write) doubles the block for those bytes and copies the payload. `bun_core::vec::reserve_for_append` asks `Vec::reserve` for 4096 bytes more where that exact fit is 1 MiB or more. Every other growth is `Vec::reserve` as before. `StreamBuffer`, `OffsetByteList`, the node:http socket buffer, `ByteVecExt::write_utf16`, the Latin-1 to UTF-8 append, `ArrayBufferSink` and the fetch request body grow through it.
|
Reproduced on a release build of main
With this PR the four rows read 1.06 to 1.07, 1.03, 1.03 and 0 MiB. The 22 new tests fail on main and pass here. PR: #46415 |
|
Updated 10:05 PM PT - Oct 21st, 2026
❌ @robobun, your commit 17c0037 has some failures in 🧪 To try this PR locally: bunx bun-pr 46415That installs a local version of the PR into your bun-46415 --bun |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 1 minute. View limit details
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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let needed = list.len().saturating_add(additional); | ||
| let exact_fit = needed > list.capacity().saturating_mul(2); | ||
| let room = if exact_fit && needed >= LARGE { | ||
| ROOM | ||
| } else { | ||
| 0 | ||
| }; |
There was a problem hiding this comment.
🟡 (optional) A payload of exactly twice the queue's capacity still forces a full copy on the next short append, same as on the base branch. grow_for_append at src/bun_core/lib.rs:466 adds room only when needed > capacity * 2, but Vec::reserve computes max(cap * 2, needed), so needed == capacity * 2 is also an exact fit with zero room. Fix: add ROOM whenever needed >= LARGE and capacity * 2 < needed + ROOM (or just reserve additional + ROOM for every needed >= LARGE, since Vec already takes the max), so the band just under a doubling keeps 4096 bytes too. Example: ArrayBufferSink.start({ highWaterMark: 1 MiB }), write(2 MiB), write("x") copies 2 MiB. [also at: src/bun_core/lib.rs:465 - nit: a payload that is exactly twice the current capacity still gets an exact fit, so the next short append copies the whole payload. grow_for_append at src/bun_core/lib.rs:465 tests needed > list.capacity().saturating_mul(2), but Vec::reserve allocates max(cap2, needed), which is also an exact fit when needed == cap2. Fix: treat needed >= cap*2 as the exact-fit case so every size Vec::reserve would fit exactly gets the 4096 bytes of room.; src/bun_core/lib.rs:466 - nit: a payload that needs exactly twice the current capacity is still fitted exactly, so the next short append copies it, same as on the base branch.]
Why this was flagged
Trigger: a list whose len + additional is exactly 2 * capacity (or within 4096 bytes below it) and at least 1 MiB, e.g. ArrayBufferSink::start at src/runtime/webcore/ArrayBufferSink.rs:31 does try_reserve_exact(1 MiB) and the first write at src/runtime/webcore/ArrayBufferSink.rs:82 is 2 MiB. In grow_for_append (src/bun_core/lib.rs:466) exact_fit is needed > list.capacity().saturating_mul(2), which is false at equality, so room is 0 and list.reserve(additional) is called. Rust's RawVec::grow_amortized sets cap = max(cap * 2, required_cap), so at equality the new capacity is exactly needed with no spare byte. The following write("x") then doubles the block and copies the whole payload, the behaviour this PR exists to remove; the base branch does the same copy. The documented rule ('a short append behind a large one fits') therefore has a hole at the doubling boundary; the fix is to request the room whenever cap * 2 < needed + ROOM and needed >= LARGE.
Verification: Triggered by a list whose len + additional lands exactly on 2 * capacity with that total >= 1 MiB, followed by a short append. At src/bun_core/lib.rs:466 needed > list.capacity().saturating_mul(2) is false at equality, so room is 0 and the call is plain list.reserve(additional). RawVec::grow_amortized picks max(cap * 2, required_cap), so the next 1-byte append copies the whole payload, as on main.
| Bun.gc(true); | ||
| // The two sends left a higher peak behind. Writing 5 to clear_refs | ||
| // sets VmHWM back to what is resident now. | ||
| fs.writeFileSync("/proc/self/clear_refs", "5"); |
There was a problem hiding this comment.
🟡 (optional) CI runs on a kernel or sandbox that refuses writes to /proc/self/clear_refs fail the new IPC test, where the base suite had no such dependency. The fixture at test/js/bun/spawn/spawn.ipc.test.ts:218 calls fs.writeFileSync("/proc/self/clear_refs", "5") with no guard. A throw there prints to stderr and exits nonzero, and expectRssDeltaBelow (test/harness.ts:485) asserts stderr is empty. The test is gated only on isLinux. Fix: skip or tolerate when the clear_refs write fails (try/catch and emit a skip marker, or measure in a fresh process so VmHWM needs no reset), so the test cannot fail for a platform reason on gVisor, CONFIG_PROC_PAGE_MONITOR=n, or hardened runners.
Why this was flagged
Trigger: the test runs on Linux where /proc/self/clear_refs is absent or EACCES/EINVAL (gVisor, CONFIG_PROC_PAGE_MONITOR disabled, some hardened containers). Entry: spawn.ipc.test.ts:200 it.skipIf(!isLinux) spawns the fixture through expectRssDeltaBelow at test/harness.ts:474. The fixture writes "5" at spawn.ipc.test.ts:218 inside try/finally with no catch, so the error is printed to stderr and the process exits 1. test/harness.ts:485 expect(stderr.trim()).toBe("") fails and test/harness.ts:488 exit code assertion fails. No other test in the tree touches clear_refs (grep), so the base suite is unaffected on those runners. The isLinux gate does not cover this: it checks the OS, not kernel config or sandbox policy. Remedy: catch the write failure and skip, or restructure the fixture so no VmHWM reset is needed.
Verification: nit — triggers when the host has no writable /proc/self/clear_refs. The fixture in test/js/bun/spawn/spawn.ipc.test.ts:218 does fs.writeFileSync("/proc/self/clear_refs", "5"); with no catch, so a throw prints to stderr and exits nonzero. The test is gated only by it.skipIf(!isLinux) and harness.ts:485 asserts expect(stderr.trim()).toBe(""). The base suite had no such dependency.
| ) -> Vec<u8> { | ||
| list.truncate(offset_into_list); | ||
| list.reserve(latin1.len()); | ||
| crate::vec::reserve_for_append(&mut list, latin1.len()); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing nit: a Latin-1 string of 1 MiB or more with more than 4096 non-ASCII characters still doubles the queue and copies the payload, the copy this PR sets out to remove. src/bun_core/lib.rs:1660 reserves only latin1.len(), so the 4096 bytes of room absorb at most 4096 one-byte expansions before list.reserve(2) at src/bun_core/lib.rs:1675 doubles the block. Fix: size the reservation by the UTF-8 length, element_length_latin1_into_utf8(latin1) from src/bun_core/lib.rs:1814, which covers every caller of the helper: StreamBuffer::write_latin1, ByteVecExt::write_latin1 and to_utf8_from_latin1.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A JS string that is 8-bit in JSC and holds ordinary French, German or Spanish text of a few MiB (thousands of accented characters) is written to Bun.spawn stdin, a FileSink, an ArrayBufferSink or a fetch() request body. Those reach StreamBuffer::write_latin1 at src/io/PipeWriter.rs:2021 or ByteVecExt::write_latin1 at src/collections/vec_ext.rs:500, both of which call allocate_latin1_into_utf8_with_list. src/bun_core/lib.rs:1660 asks reserve_for_append for latin1.len() bytes, which for a fresh or small list yields an exact fit plus 4096 bytes of room. Each non-ASCII byte becomes two bytes, so after 4096 such characters the room is gone and list.reserve(2) at src/bun_core/lib.rs:1675 doubles the block, allocating 2x the payload and copying all of it, exactly what the base branch does on the first accented character. The PR's test and table only cover a string whose single non-ASCII character is the last one, so this path is not exercised.
Verification: src/bun_core/lib.rs line 1660 crate::vec::reserve_for_append(&mut list, latin1.len()); sizes the reservation by the Latin-1 byte count, not the UTF-8 output length. Each non-ASCII byte expands to two bytes, so after 4096 such characters list.reserve(2) at line 1675 doubles the block and memcpys the whole payload. On the base branch the first non-ASCII character already doubled the block.
This branch has not been deployed
1 participant
Problem
Bun.spawnstdin:write("x")behindwrite(64 MiB)blocks for 27 to 74 ms (v1.3.14: under 0.05 ms). No user reported it.Vecfits a request for more than twice its capacity exactly. The next byte then doubles the block:mi_malloc(66889600), thenmi_realloc(p, 133779200).StreamBuffer::write(src/io/PipeWriter.rs:1954) andOffsetByteList::write(src/collections/vec_ext.rs:544) append that way since the Rust port (Rewrite Bun in Rust #30412).Fix
bun_core::vec::reserve_for_appendasks for 4096 bytes more whereVec::reservewould fit 1 MiB or more exactly. Other growths keepVec::reserve.StreamBuffer,OffsetByteList, the node:http socket buffer, the string appends,ArrayBufferSinkand the fetch body grow through it.Background
StreamBufferandOffsetByteListare aVec<u8>with a read cursor. They hold unsent bytes for pipes, IPC, fetch, S3, SQL and Redis.Downsides
FileSinkwrite: 747 to 792).Notes
What the numbers count. "main" and "PR" are release builds on Linux x64. The main binary is a build of this checkout with
src/ate53cd3d994. v1.3.14 and v1.4.0 are the release binaries. A peak is the growth ofVmHWMwhile the writes run, over the payload size. The peak does not include the payload: it is zero pages, or it is resident before the measurement starts. On main the second block lives while the copy runs and until the allocator purges it.Reach. Three rows need nothing unusual:
The other rows need a peer that does not read, or an option that is not the default.
Peak per entry point (5 interleaved runs). A cell is the ratio of the peak to the payload, or MiB where the row says so. The payload is 64 MiB unless the row gives a size.
Bun.spawnstdin, one write, nothing behind itBun.spawnstdin, thenwrite("x")child_processstdin, thenwrite("x")process.stdoutto a pipe, thenwrite("x")"x"é, then"x""x"SET, string value,enableAutoPipelining: falseSET,Uint8Arrayvalue,enableAutoPipelining: falseSET,Uint8Arrayvalue, default optionssend("x")behind a queued 32 MiB message: MiB addedfetch(), direct stream, chunked, 32 MiBfetch(), direct stream,Content-Length, 32 MiB then"x"s3file.writer(),queueSize: 2, 32 MiB then"x"Bun.spawnstdin, then 6 MiBUint8Arrayreads one payload more on main than on v1.4.0. Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021 copies a buffer argument before it serializes the command. This PR does not change that copy.child_processrow. It queues the write by reference.write("x")behind 64 MiB, 5 more runs: v1.3.14 0.03 to 0.04 ms, main 27 to 32 ms, PR 0.02 to 0.03 ms.One follow-up write behind 64 MiB in
Bun.spawnstdin (3 runs). A cell is the peak over the total, then the time of the second write. This probe resetsVmHWMbefore the first write, so it reads 0.05 lower than the table above.Queue capacity after
write(64 MiB),write("x"),write(7 MiB):The same behind a smaller first write (1 run). A cell is the queue capacity before and after the second write. On this machine the pipe takes the first 219,264 bytes, so the queue holds that much less than the write.
Traces without a short follow-up
FileSinkto/dev/null, 41 writes of 100 bytes per flush: ns per flush (minimum, then medians)ArrayBufferSinkcapacity after each write (distinct values)"x""x""x""x""x""x""x""x"A 64 MiB payload, then a larger one on the same connection (1 to 3 runs). A cell is the peak MiB of the first call, then the MiB that the second call adds. The probe resets
VmHWMbetween the calls.SET,Uint8Array,enableAutoPipelining: false, 64 then 80 MiBThe second call starts with the first block still held (64 MiB). So Postgres with 64 then 120 MiB peaks at 184 MiB in total on this branch and on v1.3.14. Main peaks at 134 MiB, in the first call: its doubled block already holds 120 MiB. By the same sum the Redis row is about 224 MiB in total on this branch and 198 on main. Above 2 times, main grows too and the totals are equal.
mi_realloccopies the old block, used or not.OffsetByteListdoes not release its block when it drains. A release on drain would remove this case, and it is not in this PR.Instructions (gdb
stepifrom entry to return). A cell is the total, then the count inside the function itself.PosixStreamingWriter<FileSink>::write, 16 bytes into the queueFileSink__write, the same write from its JS entryOffsetByteList::write, Postgres small queryArrayBufferSink__write, 16 bytesArrayBufferSink__write, 16-character ASCII stringArrayBufferSink__write, first write of a new sink (the buffer grows)FileSink__write, the write that takes the queue past 4096 bytes (the queue grows)The first five rows are appends that fit. The last two rows grow:
grow_for_appendruns in front ofVec::reserve, andgrow_and_appendin front of the copy.Size.
sizeof the release binary: text 107,380,868 on main and 107,379,332 on this branch (1,536 bytes less).OffsetByteList::writegoes from 108 to 85 bytes andStreamBuffer::writefrom 226 to 198. New:grow_for_append(94 bytes) andgrow_and_append(114 bytes). No struct changes size.Benchmarks. Release builds, interleaved rounds, ns per call. A cell is the minimum, then the median of the p50 of each round.
main2is a second run of the main binary, to show the noise.bench/needs mitata, which this machine does not have, so a small sampling loop replacesbench/runner.mjs.bench/snippets/arraybuffersink.mjs(5 rounds):The same sink with bytes and Latin-1 strings (5 rounds, not in
bench/). A "reuse" row writes to one sink and resets it. A "one-shot" row makes a sink, writes once and ends it, so every call grows the buffer once.bench/snippets/ipc-json.mjs(3 rounds of 5 samples, ms per run):Reruns with main and this branch twice each, in the order main, PR, main, PR:
In the reruns the two runs of one binary differ as much as main differs from this branch. The last two one-shot rows of the second table are the case this PR is for.
Why this rule. I built three rules. The first two ran on release builds of an earlier base (
91e7e3b140).Vecfits exactlyFileSinkget about 13% slower per flush (about 1010 ns on main, 1140 with that rule). The queue leaves its cycle of 4096 and 8192 bytes, which mimalloc serves in place.Vecfits 1 MiB or more exactly (this PR)The rule applies only to one reserve of more than 512 KiB: the list has to need 1 MiB, and more than twice its capacity. That is one append, or one Redis pipeline flush, which reserves the total of its batch. The room doubles when the block doubles later: 16 writes of 4 MiB end 65,536 bytes above main. The room stays under 0.4% of the block. Below the limit the copy is short: 829,312 bytes take 0.3 to 0.7 ms.
Also changed. The fetch arm for a body that is not chunked now reserves the UTF-8 length before it appends. A Latin-1 chunk under 1 MiB now fits exactly there. On main that arm reserved the Latin-1 length and doubled.
Not in this PR.
allocate_latin1_into_utf8_with_listreserves the Latin-1 length and then grows by 2 bytes per non-ASCII character. Its reserve now goes through the rule. That covers 4096 such characters behind a string of 1 MiB or more. Callers that size the output themselves (to_utf8_from_latin1,allocate_latin1_into_utf8) still double once for a string with a non-ASCII character.Buffer.from(string)andfs.writeFileSync(path, string)read 2.01 to 2.04 there on v1.3.14, on main and on this branch. To size from the UTF-8 length needs a second pass over the string, with its own benchmarks.ByteVecExt::write_utf16reserves 2 bytes for a lone surrogate and writes 3. A string with more than 4096 lone surrogates outgrows its reserve, as on main. strings: size the UTF-16 to UTF-8 append output in the conversion, mark copy_utf16_into_utf8_with_utf8_len unsafe #38971 (open) sizes that output in the conversion.Vec<u8>: node:netbuffered_data_for_node_net, the HTTP/2 serverwrite_buffer, the shellIOWriter. The same for byte writes to theBun.servedirect stream sink. I did not measure a copy there.Content-Length, HTTP/2, HTTP/3, upgraded connections). That arm appends to theVecdirectly. Release the consumed prefix of StreamBuffer so S3 uploads do not keep the whole payload in memory #42731 added the compaction toStreamBuffer::write.OffsetByteListkeeps its block until the connection closes. A release on drain would also remove the second case under Downsides.child_processrow. node:http: hold large res.write() payloads by reference instead of copying into uWS backpressure #34511 does that forres.write().Shared helpers.
ByteVecExt::write_utf16and the Latin-1 append (allocate_latin1_into_utf8_with_list) serve more than the queues. They also take string writes to theBun.servedirect stream sink and to HTMLRewriter. The shell output buffer andEncodedSlice::to_owned_slice_zuse them too. There, a string of 1 MiB or more gets the same 4096 bytes.Related work.
BackPressure::TAIL_ROOM). uws: BackPressure keeps room behind a growth, so framing does not copy a queued payload #45744 proposed it first. Its trailer is chunk framing. The trailers of this PR are longer: Postgres writesExecuteandSyncbehind the parameters. MySQL adds 4 bytes per 16 MiB. So the room is 4096 bytes.write_utf16into the conversion. It removes the line that this PR changes there. The PR that merges second has to put the rule on the new reserve, or the UTF-16 rows copy again.StreamBuffer. Their tests can expect exact capacities for writes of 1 MiB or more. In that case the PR that merges second has to update them.No MySQL test. The MySQL rows go through
OffsetByteList::write, which the Redis and Postgres tests cover. The stub-server tests intest/js/sqlsay that they are for fault injection only. They say that a case a real server can produce belongs indescribeWithContainer. The MariaDB on my build machine refuses the user of those tests. So I could not run a test there and did not add one. The MySQL rows come from the same probes as the other rows. The server is a local MariaDB 11.8 withmax_allowed_packetat 1 GiB.Self-review: what is not done.
write_utf16. It is wrong on main in the same way. strings: size the UTF-16 to UTF-8 append output in the conversion, mark copy_utf16_into_utf8_with_utf8_len unsafe #38971 (open) fixes it.SET, so the next command finds the buffer empty on main too.VmHWMwithout a reset first. A reset needs a write to/proc/self/clear_refs, which can fail. A test without it can miss a regression, and it cannot fail on a fixed build.VmHWM. A helper for it would touchharness.ts.Tests. Each new test fails on a release build of main and passes on a debug build of this branch.
test/js/bun/spawn/spawn-stdin-destroy.test.ts: 7 tests. Six check that the queue ofproc.stdindoes not grow. They write one byte or 4096 bytes behind 8 MiB of bytes, UTF-16 or Latin-1. One reads the peak of a child.test/js/bun/util/arraybuffersink.test.ts: 6 tests, the same rows onArrayBufferSink.test/js/bun/spawn/spawn.ipc.test.ts: a short message behind a queued 16 MiB message, in both serializations.test/js/sql/postgres-bytea-bind.test.ts: a 32 MiB bytea parameter.test/js/valkey/reliability/connection-failures.test.ts:SETof 32 MiB thenGET, with and without auto-pipelining.test/js/bun/s3/s3-networksink-leak.test.ts: one byte behind 32 MiB ins3file.writer().test/js/web/fetch/fetch-args.test.ts: a direct stream upload of 32 MiB, chunked and withContent-Length.test/js/node/http/node-http-pinned-write.test.ts: a 400 reply behind 32 MiB on an upgraded socket.Linux runs all 22. The 7 peak tests read
VmHWM, so macOS runs 15. Windows runs 11: its writer keeps the first write in a buffer of its own, so 4 more stdin rows do not apply. I cannot run Windows or macOS.The stdin tests are not in
spawn-stdin-pipe-fd-leak.test.ts, and the S3 test is not ins3-upload-memory.test.ts. On a debug build, older tests in those files take up to 4.6 s of their 5 s. One in each file timed out once.Suites run. On a debug build of this branch, the 8 files above pass. They also pass with the environment of the CI ASAN lane (
detect_leaks=1,BUN_DESTRUCT_VM_ON_EXIT=1), except one older test. That test (write() copies a WebAssembly.Memory's bytes) passed its 30 s limit there. It takes 3.4 s without that environment. On a release build of main, the same 8 files fail in the 22 new tests only.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch-args.test.ts, test/js/valkey/reliability/connection-failures.test.ts, test/js/sql/postgres-bytea-bind.test.ts, test/js/node/http/node-http-pinned-write.test.ts, test/js/bun/spawn/spawn.ipc.test.ts, test/js/bun/spawn/spawn-stdin-destroy.test.ts