Make multipart content collection linear - #7157
Conversation
Multipart.collectUint8Array reallocated and copied the full accumulator on every pull, making contentEffect quadratic in the number of upstream chunks. Buffer the chunks and concatenate once instead. Also replaces the multipart header parser's character tables with loop-built Uint8Arrays (identical entries, verified byte for byte) and adds a multipart benchmark. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 63b96fe The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions only — the collection is now linear and the header tables are byte-for-byte equivalent; two optional follow-ups below.
Reviewed changes
- Linear collection of file content (
packages/effect/src/unstable/http/Multipart.ts):collectUint8Arraynow buffers emitted chunks and concatenates once instead of copying the whole accumulator on every pull, takingFile.contentEffectfrom O(n²) to O(n) in the number of upstream chunks. - Loop-built header character tables (
packages/effect/src/unstable/http/MultipartParser/internal/headers.ts): the two large array literals were replaced withUint8Array(256)built from the RFC 7230 token/value character sets. - New benchmark (
packages/effect/benchmark/http/multipart.ts) covering the raw parser, the channel drain path, and content collection in both single-batch and per-pull modes.
I verified the two central claims directly: I extracted the old character tables from the diff and compared them against the new loop-built construction — 0/256 differing entries for both (the old 127-entry name table returned undefined for bytes ≥ 127, which the parser treats identically to the new 0), and I read Channel.runFold / runForEach / runWith to confirm the rewrite preserves semantics (same run machinery, same element granularity, same error/interruption propagation). I also ran Multipart.test.ts (10 pass), Headers.test.ts (7 pass), the package typecheck, oxlint + dprint on the changed files, and the new benchmark end-to-end (runs clean; ~1.6ms parser / ~4–5ms collect for 16MiB, matching the claimed scaling).
ℹ️ Coverage: the new collection path isn't pinned by a committed test
The only committed test that exercises contentEffect is a single 5-byte single-chunk case (test/unstable/http/HttpServerRequest.test.ts:132). The PR body describes a 300-iteration random-chunk-split fuzz that validated byte-level equivalence, but it wasn't committed. Since this rewrite is the behavioral core of the fix, a committed multi-chunk content-collection test (even a couple of fixed chunk splits, or the fuzz with a seeded RNG) in test/unstable/http/Multipart.test.ts would pin the new collector against regressions and double as the regression test for the quadratic bug this PR fixes.
Technical details
# Commit a content-collection regression test
## Affected sites
- packages/effect/test/unstable/http/Multipart.test.ts — no test feeds multiple upstream chunks through `File.contentEffect` / `collectUint8Array`
- packages/effect/test/unstable/http/HttpServerRequest.test.ts:132 — only existing `contentEffect` coverage ("hello", 5 bytes)
## Required outcome
- A test that builds a multipart body whose file payload is split across multiple chunks (e.g. arbitrary splits, or a small seeded fuzz over chunk boundaries), collects it via `part.contentEffect`, and asserts the bytes match the source exactly — including a slice that straddles the trailing boundary delimiter (the seam case).
## Suggested approach (optional)
- Reuse the `Multipart.makeChannel` + `Stream.pipeThroughChannel` idiom already present in `Multipart.test.ts`, chunking a fixed payload at varied boundaries and across the trailing boundary delimiter.@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found — the follow-up commit addresses both items from the prior review, and the incremental changes are clean.
Reviewed changes
- Simplified
collectUint8Array(packages/effect/src/unstable/http/Multipart.ts:644-667): theEffect.suspend+ capturedelements/lengthstate from the previous revision is now aChannel.runFoldinto a lazyArray<Uint8Array>accumulator with a single trailing concat pass — same O(n) behavior, no shared state across runs (LazyArg init), and identical error/interruption propagation throughrunFold's underlyingrunWithmachinery. The added JSDoc gotcha (source channel must not reuse/mutate emitted buffers) accurately documents the retained-to-completion aliasing. - Committed multi-pull regression test (
packages/effect/test/unstable/http/Multipart.test.ts:41-67): feeds three chunks throughStream.rechunk(1), splits the file content (abcdef) across chunks 1–2 and the trailing\r\n--boundary--\r\ndelimiter mid-boundary across chunks 2–3, then assertsFile.contentEffectyields the exact bytes. This directly pins the across-pulls accumulation, not just a single-batch case. Verified again at 11/11 pass. - Changeset relocated to
.changeset/root: the original placement under.changeset/pre/was in the archive directory where "Version Packages" renames consumed changesets (see the R100 renames infb75264aa); pending changesets belong at the root (as withsafe-json-schema-patterns.md). The move is a correction, not a regression. - Comment-only updates to the benchmark (clarifying which cases are single-pull controls vs. the
rechunk(1)regression guard) and to the header tables (documenting the 127–255 zero-fill rationale).
Validation this run: Multipart.test.ts 11/11, Headers.test.ts 7/7, tsc -b on packages/effect, dprint + oxlint on the four changed files — all clean.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
1 participant
Summary
Performance pass over the multipart parsing stack (
MultipartParserinternals and theMultipartwrapper), driven by CPU profiles and a new benchmark.Fix: byte collection was quadratic. The collector reallocated and copied the entire accumulator on every pull, so
File.contentEffectcost O(n²) in the number of upstream chunks. The linear collector now lives atChannel.mkUint8Array, buffers chunks, and concatenates once.Stream.mkUint8Array,Stream.mkArrayBuffer, and multipart file collection all reuse it. Collecting a 16MiB file delivered in 64KiB per-pull batches goes from 272ms to 3.5ms (~78x); all other measured paths are unchanged.The public
Channel.mkUint8Arraydocumentation records that source channels must not reuse or mutate emitted buffers before collection completes. A Channel regression covers several arrays across multiple emitted chunks, and a multipart regression collects exact file bytes throughFile.contentEffectacross multiple pulls and a split trailing-boundary seam.Simplification: header parser character tables. The two ~400-line array literals are now loop-built
Uint8Arrays. Their parser accept/reject behavior is exhaustively equivalent across all 256 byte values and benchmark-neutral. The old name table stopped at byte 126, so bytes 127–255 changed from missing entries to zeroes; both representations reject those bytes because the parser accepts only entries equal to1.Added
packages/effect/benchmark/http/multipart.tscovering the raw parser, channel drain, and content collection. The non-streaming collection cases are single-pull controls becauseStream.fromArraybatches their chunks; therechunk(1)streaming case is the regression guard for accumulation across pulls.Profiled but left alone
Buffer.indexOfboundary scan and runs at ~17GB/s; instrumentation showed the chunk-seam concat path copies only ~0.1MiB per 16MiB parse, so no rewrite is warranted.Validation
pnpm checkchangeset status --since origin/main🤖 Generated with Claude Code