From 1760f7eea0059a623bb05242ab62d090e87ebb6e Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 20:56:16 -0700 Subject: [PATCH 1/3] chore(quest): claim quest/m1/release-perf/js-frames.md Quest-Claim: 251688a7054d1526 From 6791d71e987dd0653455de7311d125e3f57bb8d9 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 21:19:29 -0700 Subject: [PATCH 2/3] perf(net): drop a finished stream's reader instead of releasing its lock Releasing the reader lock of a stream that already ended replaces its closed promise with a rejected one, which builds a TypeError for every group stream. In Chromium that cost about 5 us per stream, which put the js/net frame benchmark 1.2 to 1.5x behind release. Dropping the reference is enough to keep a later stop from cancelling. Co-Authored-By: Claude Opus 5.5 --- js/net/src/stream.test.ts | 26 ++++++++++++++++++++ js/net/src/stream.ts | 8 +++---- quest/m1/release-perf/README.md | 12 ++++++---- quest/m1/release-perf/js-frames.md | 38 ------------------------------ 4 files changed, 37 insertions(+), 47 deletions(-) delete mode 100644 quest/m1/release-perf/js-frames.md diff --git a/js/net/src/stream.test.ts b/js/net/src/stream.test.ts index d166ac7e6c..89ab2bbd44 100644 --- a/js/net/src/stream.test.ts +++ b/js/net/src/stream.test.ts @@ -538,6 +538,32 @@ test("closed is stable, so racing it per frame does not allocate", async () => { expect(reader.closed).toBe(reader.closed); }); +test("reading to FIN builds no error", async () => { + // Fixtures built outside the window, and kept alive so an error they retain stays counted. + const readers = Array.from( + { length: 32 }, + () => + new Reader( + new ReadableStream({ + start(controller) { + controller.enqueue(new Uint8Array([7])); + controller.close(); + }, + }), + undefined, + QUIC, + ), + ); + const before = heapStats().objectTypeCounts; + for (const reader of readers) { + expect(await reader.u8()).toBe(7); + expect(await reader.done()).toBe(true); + } + const after = heapStats().objectTypeCounts; + expect(after.TypeError ?? 0).toBeLessThanOrEqual(before.TypeError ?? 0); + expect(after.Error ?? 0).toBeLessThanOrEqual(before.Error ?? 0); +}); + test("stop after FIN allocates nothing and sends nothing", async () => { const cancelled: unknown[] = []; const reader = new Reader( diff --git a/js/net/src/stream.ts b/js/net/src/stream.ts index f33f1945ce..92409a43da 100644 --- a/js/net/src/stream.ts +++ b/js/net/src/stream.ts @@ -314,11 +314,9 @@ export class Reader { if (result.done) { // The transport already finished the stream. Drop the reader so a later stop - // neither builds a cancel error nor sends a reset the peer will ignore. - if (this.#reader === reader) { - this.#reader = undefined; - reader.releaseLock(); - } + // neither builds a cancel error nor sends a reset the peer will ignore. Dropped, + // not released: releasing a closed stream's lock builds a TypeError per stream. + if (this.#reader === reader) this.#reader = undefined; return false; } diff --git a/quest/m1/release-perf/README.md b/quest/m1/release-perf/README.md index 7f1f099130..5b2e8c4d49 100644 --- a/quest/m1/release-perf/README.md +++ b/quest/m1/release-perf/README.md @@ -8,8 +8,9 @@ is fixed. Cost that pays for a feature `main` deliberately added is accepted, and the reason is written here. This epic's own work: once every child has landed, run the full main against -release comparison again with `just bench origin/release`, and confirm nothing -new regressed. Then add a release-baseline step to the recurring +release comparison again with `just bench origin/release`, plus the JS frame +path in Chromium with `bench/browser/micro.ts`, and confirm nothing new +regressed. Then add a release-baseline step to the recurring [performance audit](/quest/m1/audit-perf.md), so every later cut repeats this comparison. @@ -102,7 +103,11 @@ and 12% less on fanout, with the same delivery. - kio `contended_notify` and `contended_log` at 16 threads: 4 to 5x. These are contention benches on a busy host, so they are likely noise. - JS `net/bench/frames.ts` on bun: lite rows 1.2 to 1.3x, IETF rows 1.3 to - 2.2x ([JS frames](/quest/m1/release-perf/js-frames.md)). + 2.2x. Fixed in Chromium: + [#6296](https://github.com/moq-dev/moq/pull/6296) removed a retained-history + scan, and [#6479](https://github.com/moq-dev/moq/pull/6479) stopped + releasing a finished stream's reader lock, which built a `TypeError` per + group stream. Every row then sits within the A/A spread of release. The raw report lived in a scratch directory that has since been removed. `just bench ` now pairs full Criterion IDs; point `MOQ_BENCH_ARTIFACTS` @@ -112,7 +117,6 @@ outside the worktree to keep the next one. - [Sparse gaps](/quest/m1/release-perf/sparse-gaps.md) - an arrival and a gap flush cost O(log gaps), not O(gaps) - [Re-check](/quest/m1/release-perf/recheck.md) - the screen-only moves are re-measured with instruction counts, and the ones that hold are fixed or accepted -- [JS frames](/quest/m1/release-perf/js-frames.md) - the js/net frame path is no slower than release in a browser, or the gap is accepted ## Related diff --git a/quest/m1/release-perf/js-frames.md b/quest/m1/release-perf/js-frames.md deleted file mode 100644 index 2ad8f52472..0000000000 --- a/quest/m1/release-perf/js-frames.md +++ /dev/null @@ -1,38 +0,0 @@ -# [M] JS frames - -## Goal - -The js/net frame path (`js/net/bench/frames.ts`) is no slower than -`release` when measured in a browser, or the gap is accepted with a reason -recorded in the [epic](/quest/m1/release-perf/README.md). - -## Plan - -[PR #6296](https://github.com/moq-dev/moq/pull/6296) removed an accidental -retained-history scan on accepted untimed tracks. Unknown metadata still checks -buffered timestamps, and timed and wall-clock expiry keep their behavior. A -retained-groups x publishers x readers benchmark runs in Chromium and nightly. - -Measured in headless Chromium 156.0.8078.4, with one full warmup per persistent -page, three alternating rounds, identical current/A/A bundles, and unchanged -`frames.ts` on release and main. Small-chunk IETF medians improve 32 to 41%: -16 B frames / 24 B chunks 4295 to 2860 ns/frame (release 2545), and 1000 B / -1008 B 6060 to 4140 ns/frame (release 3835). A/A ratios for those rows are -0.99 and 1.04. The retained-history sweep checks each live payload, excludes -replay/setup, and parks reads before writing. With 1024 retained groups, all -publisher/reader shapes improve in every round; medians fall from 15.6 to -63.6 us/frame to 1.9 to 2.9 us/frame. Small fixtures remain noisy. - -The original Signal.race prototype had no consistent browser benefit and was -dropped. Profiles show per-write idle activity clock reads and expiry work on -main that release lacks, but that does not establish every remaining gap as -intended cost. Lite and some batched IETF rows remain above release, with noisy -A/A controls. Do not call parity complete or accept those gaps implicitly. - -Decided 2026-10-22: land the scoped scan fix and its browser sweep in -[PR #6296](https://github.com/moq-dev/moq/pull/6296), without accepting the -remaining gap, which is unattributed and noisy. This quest stays open for: - -- Attribute the remaining stable release gaps, then fix accidental cost or ask - the maintainer to accept the intended feature cost with a reason in the epic. -- Re-run the browser comparison after the other release-perf children land. From 11cb6145883617be208d0fe6b5f0d363d8b152ee Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 22:27:48 -0700 Subject: [PATCH 3/3] fix(net): stop retaining a finished stream through Reader Reader kept its ReadableStream only to call getReader(), so after FIN the still-locked stream held the dropped reader alive for as long as the Reader was reachable. Co-Authored-By: Claude Opus 5.5 --- js/net/src/stream.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/js/net/src/stream.ts b/js/net/src/stream.ts index 92409a43da..d770bb8ebb 100644 --- a/js/net/src/stream.ts +++ b/js/net/src/stream.ts @@ -278,7 +278,7 @@ export class Reader { #buffer: Uint8Array; #chunks: Uint8Array[] = []; #chunked = 0; // bytes across #chunks - #stream?: ReadableStream; // if undefined, the buffer is consumed then EOF + // Undefined once the stream finishes, or without a stream: the buffer is consumed then EOF. #reader?: ReadableStreamDefaultReader; #closed?: Promise; // The decode that last ran short and how far, so a retry can wait for those bytes. @@ -294,8 +294,7 @@ export class Reader { version: StreamVersion, ) { this.#buffer = buffer ?? new Uint8Array(); - this.#stream = stream; - this.#reader = this.#stream?.getReader(); + this.#reader = stream?.getReader(); this.version = version; }