Skip to content

perf(h2,overload): fix write-queue sharding + Reap GC storm + realize inline-response bypass (#406 #407 #408)#412

Closed
FumingPower3925 wants to merge 3 commits into
mainfrom
fix/h2-sharding-reap-inline-v158
Closed

perf(h2,overload): fix write-queue sharding + Reap GC storm + realize inline-response bypass (#406 #407 #408)#412
FumingPower3925 wants to merge 3 commits into
mainfrom
fix/h2-sharding-reap-inline-v158

Conversation

@FumingPower3925

Copy link
Copy Markdown
Contributor

Summary

Three engine perf/correctness issues surfaced by review, fixed for v1.5.8. Each
comes with a regression test that fails on the old code and passes on the fix.
Independent of the WS-crash work (#411) — separate branch off main.

Closes #406, #407, #408.

#406 — h2 write queue used only 2 of its 4 shards

h2ShardedQueue.Enqueue picked a shard with streamID % h2QueueShards (=4).
Client HTTP/2 request streams are always odd (RFC 9113 §5.1.1) and Celeris does
no server push, so odd % 4 ∈ {1,3} — shards 0 and 2 were permanently dead and
the "4-shard" queue behaved like 2.

Fix: (streamID>>1) % h2QueueShards — shift off the constant low bit so odd
IDs 1,3,5,7,… spread across all shards. Still a pure function of streamID, so
per-stream HEADERS-before-DATA FIFO + single-writer drain are unchanged; only
the distribution improves. Regression test asserts even spread + stream affinity
(fails on %4: shards 0,2 get 0 buffers).

#407 — overload Reap forced a GC on every poll tick

StageReap ran runtime.GC() on every poll tick while the signal sat in the
Reap band (default CPU 0.80–0.85), not once on entry. Under sustained moderate
load that's a full STW GC every PollInterval (default 1s) — repeated capacity
reclaim precisely when the server is already loaded.

Fix: track prevStage (goroutine-local) and fire the GC only on
prev != StageReap && new == StageReap; re-entry re-fires. Gated behind
EnableReap (opt-in, default off). Also documents ReapAggressiveness==2 as
reserved (it currently behaves identically to 1). Regression test asserts the
NumForcedGC delta while lingering in the band is ≤1 (29 on the old code).

#408 — h2 inline-response outBuf bypass was wired but never used

h2InlineResponseAdapter / Processor.InlineWriter were fully built but
executeHandlerInline still set ResponseWriter = connWriter, so inline GET/HEAD
END_STREAM responses paid the sharded-queue mutex, a pooled buffer, and an
eventfd self-wake syscall on the already-awake event-loop thread, then drained
back into outBuf.

Fix: route inline execution through InlineWriter so buffered responses append
straight to outBuf. Safe because inline runs on the event-loop thread under
H2State.mu.

⚠️ The naive one-line swap would have broken SSE/chunked-over-H2:
Context.StreamWriter() type-asserts ResponseWriter.(stream.Streamer) and 500s
on nil, and h2InlineResponseAdapter was not a Streamer — a reachable regression
on a default GET /events route that neither the unit tests nor a throughput bench
would catch. So the inline adapter now implements Streamer, delegating
WriteHeader/Write/Flush/Close to the queue
(streaming may run on a detached
goroutine, which must not write outBuf directly). Buffered → direct outBuf (the
win); streaming → queue (safe). A nil-guard falls back to connWriter for
Processors built without the conn layer.

End-to-end regression test asserts the inline writer is both the direct-outBuf
adapter (optimization applied) and a stream.Streamer (SSE-safe).

Verification

…is#406

Client HTTP/2 request streams are always odd (RFC 9113 §5.1.1) and Celeris does
no server push, so streamID%h2QueueShards (=4) only ever hit shards {1,3} and
left 0,2 dead — the 4-shard queue was effectively 2 shards. Shift off the
constant low bit so odd IDs 1,3,5,7,… spread across all shards. Pure function of
streamID, so per-stream HEADERS-before-DATA FIFO + single-writer drain are
unchanged; only the shard distribution improves.

Adds a regression test (fails on %4: shards 0,2 get 0 bufs).
…ris#407

StageReap forced a full runtime.GC() on every poll tick while the signal sat in
the Reap band (default CPU 0.80-0.85), not once on the transition into Reap.
Under sustained moderate load that runs a STW GC every PollInterval (default 1s)
— repeated capacity reclaim precisely when the server is already loaded, doing
more harm (GC CPU + STW pauses) than good.

Track prevStage (goroutine-local; run() is the sole writer of the stage atomic)
and fire runtime.GC() only on prev!=StageReap && new==StageReap. Re-entry
(Reap->Reorder->Reap) correctly re-fires. Gated behind EnableReap (opt-in,
default off). Also documents ReapAggressiveness==2 as reserved (it currently
behaves identically to 1; no pool-drain primitive is wired).

Adds a NumForcedGC-delta regression test (29 GCs while lingering pre-fix, <=1
post-fix).
The direct-to-outBuf inline path (h2InlineResponseAdapter / Processor.InlineWriter)
was fully wired but never read: executeHandlerInline still set ResponseWriter =
connWriter, so inline GET/HEAD END_STREAM responses paid the sharded-queue mutex,
a pooled buffer, and an eventfd self-wake syscall (Enqueue on the same event-loop
thread that is already awake), then drained back into outBuf.

Route inline execution through InlineWriter so buffered responses (WriteResponse)
append straight to outBuf, skipping all of that. Safe because inline runs ON the
event-loop thread under H2State.mu — the same invariant control frames rely on.

The naive swap would have broken SSE/chunked-over-H2: Context.StreamWriter()
type-asserts ResponseWriter.(stream.Streamer) and 500s on nil, and the inline
adapter was not a Streamer. Fix: h2InlineResponseAdapter now implements Streamer,
delegating WriteHeader/Write/Flush/Close to the queue adapter (streaming may run
on a detached goroutine, which must NOT write outBuf directly). So buffered ->
direct outBuf (the win), streaming -> queue (safe). nil-guard falls back to
connWriter for Processors built without the conn layer (unit tests).

Adds an end-to-end regression test asserting the inline writer is both the
direct-outBuf adapter AND a stream.Streamer.
@FumingPower3925

Copy link
Copy Markdown
Contributor Author

Merged into #411 — the combined v1.5.8 PR now carries these three H2/overload fixes (#406/#407/#408) alongside the WS-crash + Context-recycle fixes. Same commits (d7ade0f/bcdc8f1/32e51d8), no file overlap. Closing in favor of the single release PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

h2 write queue: streamID % 4 uses only 2 of 4 shards (client stream IDs are always odd)

1 participant