perf: stop copying request bodies twice - #369
Closed
nigrosimone wants to merge 1 commit into
Closed
Conversation
uWS neuters its ArrayBuffer once the data callback returns, so every chunk has to be copied out of it - and then Buffer.concat copied the whole body a second time to make it contiguous. A 512 KiB body was allocated and memcpy'd twice per request, plus one allocation per chunk for the garbage collector. When content-length is known and the body is not being inflated, the final size is known before the first chunk arrives, so the chunks are copied straight into a single buffer and the body is copied once. Bodies without a content-length, compressed bodies, and bodies over the preallocation cap keep the old path, which also now skips the concat when the whole body arrived in one chunk - the common case for small requests. The cap exists so a client that declares a body and never sends it cannot pin more memory than one that actually sends a body that size; a content-length above options.limit was already rejected before any of this. Adds a test covering the paths this splits into: single chunk, several chunks with a content-length, a body past the cap, a chunked body with no content-length, a gzipped body, and a binary round trip that would catch a body assembled at the wrong offset.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every request body is currently copied twice before it reaches the parser.
uWS neuters its
ArrayBufferonce the data callback returns, so each chunk has to be copied out of itabs.push(Buffer.from(buf))inonData. ThenonEndcallsBuffer.concat(abs), which allocates the full body again and copies all of it a second time. A 512 KiB request means two 512 KiB allocations and two full memcpys, plus one allocation per chunk for the garbage collector to deal with.What changed
When
content-lengthis known and the body is not being inflated, the final size is known before the first chunk arrives. The chunks are copied straight into one preallocated buffer at the right offset, andonEndreturns it as-is. One copy instead of two, and no per-chunk garbage.Three cases keep the previous path:
content-length: a chunked body's size is only known at the endcontent-encodingset:content-lengthdescribes the compressed size, not the parsed oneThat path also got a small fix of its own: when the whole body arrived in a single chunk, which is the common case for small requests, the copy already made is the body, so the concat is skipped.
On the cap
MAX_PREALLOCATED_BODYis 1 MiB. Without a cap, a client could declare a largecontent-length, send one byte, and make the server allocate the whole declared size, today that costs it almost nothing. With the cap, a client that declares a body and never sends it cannot pin more memory than one that actually sends a body that size, which it could do anyway. Acontent-lengthaboveoptions.limitis already rejected before any of this, so the preallocation is also bounded by whatever the application configured.Measurements
Local A/B, baseline
srcagainst modifiedsrcon the same machine, 5 paired runs, median of per-run ratios:The 512 KiB row is the one to trust: all five pairs favour the change and they sit in a narrow band, 1.08–1.18. At the smaller sizes the gain is real but close to this machine's noise floor of about ±6%, so read those as directional.
middlewares/body-json-512kbon CI is one of the rows bounded byJSON.parse, the arithmetic ceiling there is around 1.02x on the ratio against express, because express pays the same parse. This does not change that. What it changes is uExpress's own absolute number, which is what the local A/B measures.Unrelated thing found while writing the test
With
express.raw()and an empty body, express setsreq.bodyto an empty Buffer while uExpress leaves it as the default empty object, socrypto.createHash().update(req.body)throws on uExpress and works on express. It reproduces onmainwithout this branch,type-isreports no body atcontent-length: 0, so the parser returns early and never assigns. Not touched here; worth a separate look.