Conversation
A short send leaves the rest of a node:net write in `NewSocket::buffered_data_for_node_net`. The three places that take sent bytes out of that queue moved every unsent byte to the front of the Vec after each partial send. For a tail of C bytes that leaves in steps of s bytes that is C * C / (2 * s) bytes moved. The queue is now `PendingWrites`, a newtype over `bun_io::StreamBuffer`. `consume` advances the cursor. The queue frees its allocation when it empties, and an append drops the sent prefix once it is at least as large as the unsent bytes (the rule StreamBuffer got in #42731).
|
Status of #44219 How the cost was reproduced:
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughThe socket runtime adds ChangesPending-write queue
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change makes node:net partial-write draining cheaper without changing byte ordering. Tests cover short sends over TCP and TLS. No concrete merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the three collapsed call sites in src/runtime/socket/socket_body.rs: consume(n) reproduces each old branch (n >= len frees, n < len keeps the tail) including the writev case where written exceeds the queue length, and at the TLS site (:2819) remaining_in_input_data borrows the caller's input buffer rather than the queue, so advancing the cursor cannot shift bytes under it.
Extended reasoning...
The change swaps the Vec node:net pending-write queue for a StreamBuffer-backed cursor newtype and rewires three drain sites plus a test-only replay probe; it touches no auth, crypto, or input-parsing surface. One inline finding (the 2x allocation growth behind a partly sent tail) is already posted, so this note only records the semantic-equivalence checks on the call sites that were ruled out.
Problem
socket.end(Buffer.alloc(64 << 20))over a unix socket takes 1,107 ms (Node v26.3.0: 18 ms). No user reported it.internal_flush(src/runtime/socket/socket_body.rs:3156) andwrite_or_end_buffered(:2822,:2864) move all unsent bytes to the front ofbuffered_data_for_node_net.Fix
PendingWrites, a newtype overbun_io::StreamBuffer.consumeadvances a cursor and moves no bytes.test/internal/socket-pending-writes.test.ts,test/js/node/net/net-syscall-fault.test.ts. On main the first cannot load, the second fails 2 of 8 tests.Background
us_socket_writeis onesend(2). The caller sends the rest on writable events.StreamBufferis aVec<u8>with a cursor. An append drops the sent prefix once it is as large as the unsent bytes (Release the consumed prefix of StreamBuffer so S3 uploads do not keep the whole payload in memory #42731).{buf, head}queue (one more copy of that rule) and holding the caller's buffer (it changes which bytes a write-then-mutate sends).Downsides
NewSocket: 232 to 240 bytes (same allocator class). A whole write: 954 to 956 instructions. The binary: +4,419 bytes.drain.Notes
Sender CPU, release builds
socket.end(Buffer.alloc(N)), linux-x64, loopback, reader is Node v26.3.0 in a second process. The numbers are the CPU time of the sender (user plus system) fromend()to its callback, in ms. Median of 7 interleaved rounds. main is 9f70da0, this PR is bd4967d.A sender that writes 64 MiB in 64 KiB chunks and waits for
drain(15 rounds): unix socket 42.1 ms on main, 42.2 ms on main again, 42.7 ms with this PR. TCP 34.8, 33.3 and 33.2 ms.The peer decides the size of a send.
end()of 64 MiB over TCP to a reader that setsSO_RCVBUFto 2,048 and reads as fast as it can (3 rounds): 528.1 ms on main, 110.5 ms with this PR, 65.5 ms on Node.Not measured: many connections that drain at the same time, and a link with a small congestion window.
The machine had a load average near 380 on 16 cores, so these are CPU times and not wall times.
Scripts for the sender CPU table
sender.js <unix|tcp|tls> <path or port> <bytes> <end|drain>, run with each build:reader.js <unix|tcp|tls> <path>, run with Node:Bytes moved
Debug builds, TCP loopback, sender
net.connect(port, () => sock.end(payload)), every send clamped withsocketFaultInjection.set({ syscall: "send", action: "short", bytes, repeat: -1 }).onWritable buffered_data_for_node_net <len>lines ofBUN_DEBUG_Socket=1, measured at 36cd151. The three sites are the same on 9f70da0. On main that length is what the flush just moved. With this PR the line still prints the queue length, and it no longer means bytes moved.self.list.drain(..self.cursor)inStreamBuffer::compact).Cost for a socket that never queues
size_of::<NewSocket>(): release 232 to 240 bytes (both in mimalloc's 256-byte class), debug 504 to 512.estimateShallowMemoryUsageOf(socket._handle)of an idle socket: 329 to 337.write()that one send takes whole, counted with gdbstepion the release builds:TCPSocketPrototype__writeBufferedand its callees 954 to 956 instructions, 20 to 20 calls. ThebytesWrittengetter 36 to 37 instructions, 0 calls. The queue length is now two loads and a compare.size): text 80,666,326 to 80,670,745 bytes, data and bss the same. 3,090 of the 4,419 bytes are the test probe.pendingWritesReplayProbeinbun:internal-for-testing. No new rows insockets.classes.ts.perf,valgrind,strace,bloaty.Tests
test/internal/socket-pending-writes.test.tsruns on every lane. It replays schedules of appends and drains on aPendingWritesand reads how many unsent bytes changed address.net-syscall-fault.test.tsneeds a build that can clamp a send, so it runs wheresocketFaultInjection.available()is true. It is skipped on Windows like the rest of that file.bytesWritten.consume, no clamp inconsume,reset()in place of a free, a length that counts the sent prefix, and a count that is off by one at each of the three drain sites.*.test.*file intest/js/node/net/,test/js/node/tls/tls-syscall-fault.test.ts,node-tls-connect,node-tls-server,node-tls-upgrade,node-tls-raw-end,node-tls-duplex-end-verify,test/js/bun/net/socket.test.ts,socket-syscall-fault.test.ts,node-http-syscall-fault.test.ts, and fromtest/js/node/test/parallel:test-net-bytes-written-large,test-net-write-slow,test-net-throttle,test-net-write-fully-async-buffer,test-net-write-fully-async-hex-string,test-tls-buffersize,test-tls-fast-writing,test-tls-connect-stream-writes. The machine was under heavy load, so tests that spawn processes hit their 5 s timeout at random. Every failure also shows with main'ssrc/, or goes away when the test runs alone.bun run rust:check-all: 12 of 12 targets.Windows
Not measured. The queue is the same type on every platform and transport. The writev arm is
#[cfg(unix)], so on Windows a write behind a tail takes the append-then-send arm.Not changed
drain_frontand forcopy_within(.., 0).src/runtime/cli/multi_run.rs:382:bun run --paralleland--sequentialshift the line buffer once per line.src/runtime/node/quic/stream.rs:489:drain_outboundcopies the queued bytes before each write to lsquic. node:quic: write the outbound queue to lsquic in place instead of copying it every on_write #37491 is open for it and has conflicts.Buffer.concat([held, chunk])per chunk:src/js/builtins/ConsoleObject.ts:33and:76,src/js/internal/debugger.ts:40,src/js/node/net.ts:1970.write_or_endstill copies the unsent tail once into the queue. An empty queue frees its allocation, as before. node:net: hold queued _writev chunks by reference instead of concat + native copy #35940 (open) holds_writevchunks by reference, and node:http does so since node:http: hold large res.write() payloads by reference instead of copying into uWS backpressure #34511. A single large chunk still reaches this queue with node:net: hold queued _writev chunks by reference instead of concat + native copy #35940.socket.setTimeout()does not fire while the queue holds bytes, before and after this PR. socket.setTimeout() never fires when the peer stops reading and the write queue stalls #42403 tracks that.bun test --parallelchannel and in the uWS backpressure buffer.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/node/net/net-syscall-fault.test.ts