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
Closed
Conversation
…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.
Contributor
Author
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.
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.Enqueuepicked a shard withstreamID % 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 andthe "4-shard" queue behaved like 2.
Fix:
(streamID>>1) % h2QueueShards— shift off the constant low bit so oddIDs 1,3,5,7,… spread across all shards. Still a pure function of
streamID, soper-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
StageReapranruntime.GC()on every poll tick while the signal sat in theReap 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 capacityreclaim precisely when the server is already loaded.
Fix: track
prevStage(goroutine-local) and fire the GC only onprev != StageReap && new == StageReap; re-entry re-fires. Gated behindEnableReap(opt-in, default off). Also documentsReapAggressiveness==2asreserved (it currently behaves identically to 1). Regression test asserts the
NumForcedGCdelta 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.InlineWriterwere fully built butexecuteHandlerInlinestill setResponseWriter = connWriter, so inline GET/HEADEND_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
InlineWriterso buffered responses appendstraight to
outBuf. Safe because inline runs on the event-loop thread underH2State.mu.Context.StreamWriter()type-assertsResponseWriter.(stream.Streamer)and 500son nil, and
h2InlineResponseAdapterwas not aStreamer— a reachable regressionon a default
GET /eventsroute that neither the unit tests nor a throughput benchwould catch. So the inline adapter now implements
Streamer, delegatingWriteHeader/Write/Flush/Closeto the queue (streaming may run on a detachedgoroutine, which must not write
outBufdirectly). Buffered → direct outBuf (thewin); streaming → queue (safe). A nil-guard falls back to
connWriterforProcessors 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
go test -race ./internal/conn/... ./protocol/h2/... ./middleware/overload/ ./middleware/sse/— green.go build ./...+go vetclean.