DownloadBuffer's per-chunk ByteURL allocation: inherent, not a PR #354 regression

[o-nnerb]

Context

While reviewing PR #354 (isolating RequestDL from NIOCore/NIOSSL behind a URLSession-only trait) for performance regressions, one finding flagged PropertyMockedTask.mockBodyResponse — the code that feeds a .mockedTask response body into Internals.DownloadBuffer — for doing extra work per streamed chunk:

1. Forcing every chunk through Internals.Bytes.asData() by iterating RequestBody's public, Data-yielding AsyncSequence conformance instead of its internal, Internals.Bytes-native bytesSequence.
2. Awaiting Internals.DataBuffer(_ url: Internals.ByteURL) async per chunk, even though a ByteURL's writtenBytes is a lock read, not I/O — there's nothing to actually suspend for.
3. Allocating a fresh Internals.ByteURL per chunk.

(1) and (2) were real, avoidable costs and are fixed on that branch: the loop now drains body.bytesSequence directly, and the DataBuffer construction is routed through a small non-async helper so Swift's async-overload resolution picks Internals.DataBuffer's synchronous, suspension-free init(_ url:) instead of committing to the async one (which it does for any call made from an async context, await or not — only a genuinely non-async context leaves that overload out of the running).

(3) is what this discussion is about. It's not fixed, and — after digging in — I don't think it should be "fixed" as an isolated performance tweak. Writing this up so the reasoning has a home if the topic resurfaces.

Why the ByteURL can't be reused across chunks

The obvious next move, by analogy with the same PR's PortableZlibCompressorStream fix (hoisting its 32 KiB output buffer to a stored property reused across compress() calls instead of allocating fresh each time), would be to hoist a single ByteURL onto PropertyMockedTask and reuse it for every chunk.

That doesn't work here, and the difference is worth spelling out. PortableZlibCompressorStream's reused buffer is safe because each compress() call's output is fully copied out (via chunk.prefix(produced)) into the caller's accumulator before the next call reuses the buffer — nothing downstream ever holds a reference to the buffer itself, only to bytes already copied out of it.

Internals.ByteURL is different: its identity is its storage. buffer.append(dataBuffer) on Internals.DownloadBuffer doesn't copy the chunk's bytes out — it queues the DataBuffer (a cursor over the shared ByteURL-backed store) for whatever eventually iterates the download stream, which may run arbitrarily later relative to when the chunk was produced. If the same ByteURL backed the next chunk too, byteURL.replace(with:) would overwrite the first chunk's bytes in place — silently corrupting or truncating a chunk still sitting in the queue, not yet read. There's no "the reader already took what it needed" guarantee the way there is for the zlib buffer's synchronous, single-threaded produce-then-copy-then-reuse cycle.

So: one independent, uniquely-identified store per chunk is load-bearing here, not an oversight.

This already matches the real network path — not a PR #354 regression

The interesting bit: this is exactly what the production .nio download-receiving code already does, and it predates this PR entirely.

Internals.ClientResponseReceiver.didReceiveBodyPart — the HTTPClientResponseDelegate callback that feeds real downloaded bytes into the same Internals.DownloadBuffer — does:

let dataBuffer = Internals.DataBuffer(Internals.ByteURL(buffer))

One ByteURL per HTTPClient.Task-delivered body part, same reasoning: each part is queued independently and may be consumed later, so it needs its own store. git log on that file shows only a #if canImport(NIOCore) wrapper added around it for this PR's trait-gating work — the per-part ByteURL allocation itself hasn't changed.

So once (1) and (2) above were fixed, PropertyMockedTask's mocked path pays exactly the same per-chunk cost the real network path already accepts for the same reason. It's not a regression to chase down; it's the existing DownloadBuffer chunk model doing what it's designed to do.

What would actually change this

Removing the per-chunk allocation entirely would mean changing how Internals.DownloadBuffer/Internals.Buffer represent queued chunks — e.g., some pooled/reusable-storage scheme with explicit ownership handoff once a chunk is fully drained. That's a real design question (with its own correctness surface: ordering guarantees, Internals.CacheStream tee-ing, cancellation mid-read, the AsyncLock.Watchdog instrumentation Internals.Buffer.Storage already carries), and it would need to apply to the live network path too, not just the mock one — there's no reason for the two to diverge, and the mock path exists specifically to mirror production behavior. Out of scope for a performance-regression pass; worth its own discussion if someone wants to pursue it, but nothing in PR #354 makes it more or less urgent than it already was.

Bottom line

• PropertyMockedTask's mocked-response streaming had two real, avoidable inefficiencies relative to the .nio production path — both fixed.
• The remaining per-chunk ByteURL allocation is inherent to DownloadBuffer's chunk-queue model, already paid by the real network path (Internals.ClientResponseReceiver.didReceiveBodyPart), and predates this PR.
• No further action tracked against PR Isolate RequestDL from NIOCore/NIOSSL behind a URLSession-only SPM trait #354 for this. Flagging here in case it comes up again in a future review, so the reasoning doesn't have to be re-derived from scratch.