Repository navigation
Conversation
…t stream, so a warm HTTP/3 response allocates none of it Every stream QuicEngineConnection sent on got a new OutStream with its own Queue, and the queue's array grew as chunks queued: about 185 B per HTTP/3 response, all of ioxide.nghttp3's garbage. PurgeOutStream now resets it and keeps it on the connection, and SendStream takes one from there. A queue that grew past 64 slots is trimmed first, so a stream of many small chunks leaves no large array behind.
…ts test runs in a quarter of the time From review: - The pool keeps at most 32 OutStreams, so a burst of up to 1024 concurrent streams no longer stays on an idle connection, and Destroy clears it. - A queue past 64 slots is trimmed to 64 rather than to nothing, as the comment says, so the next large stream grows it once rather than from empty. - The test makes 16 requests instead of 48: the test client spends about 1.5 s on each, and the 48 took 61-75 s in CI against a 120 s watchdog. It checks that both marks come from the reactor's thread. Main still fails it at 184 B per response.
…and takes half as long It allowed under 64 B per response over 12 responses, so four of them could build fresh send state and it still passed. It now allows under 184 B in total, one stream's send state, over 4 measured responses: main fails it at 736 B, and the test takes about 13 s instead of 27 s. Also from review: the purge comment moves to the reset it explains and names the callers that can still hold the stream (SendStream and ReplayOut, not PumpOut); comments are cut to one line.
This branch has not been deployed
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.
What was wrong
QuicEngineConnectionsent on got a newOutStream(Connection/QuicEngineConnection.SendStream.cs), with its ownQueue<(nint Ptr, int Len)>whose array grew as chunks queued.ioxide.nghttp3, it was all of its per-request garbage.The fix
PurgeOutStreamdrops a stream's send state once the stream is closed. It now resets theOutStreamand keeps it on the connection (_outStreamPool).SendStreamtakes one from there before it makes a new one.OutStreamthrough_outStreamsby its id, so the old id finds nothing after the purge.PumpOutcan purge the stream it is pumping, and returns right after.SendStreamandReplayOutthen still hold it, and see a reset stream with nothing left to send._outPendinguntil the nextReplayOut, which finds no stream for it and drops it, as on main.OutStreams, so a burst of up to 1024 concurrent streams does not stay on an idle connection.Destroyclears the pool.Behaviour
OutStreams; the live ones are bounded by the peer's stream limit, as before.API
None.
Tests
New in the E2E suite:
quic: a warm connection's responses allocate no send-stream state (nghttp3)(QuicSendStreamTests.cs).ioxide.nghttp3makes no other garbage per request, so what it measures is the send state alone.Also new:
quic: a burst leaves the send state of at most 32 streams, with no queue past 64 slots.Against main's
ioxide.ngtcp2(the test only):With the fix:
Review
An 8-agent review of this PR and its siblings found no correctness problem in the reuse itself. It found three things, fixed in 79bb5ee:
Bench
Re-measured after the review changes: allocation per request is unchanged.
Nghttp3Bufferedallocates 0.1 B (base 182 B), andManagedBuffered1,695 B (base 1,882 B). The tables below are the first run.Setup: the same as #486.
h2load -c 32 -m 16 -t 1(with--alpn-list=h3for HTTP/3) pinned to CPU 2, on separate physical cores.Http3/Nghttp3BufferedHttp3/ManagedBufferedAllocations, from
dotnet-countersover 20 s under the same load:Nghttp3Buffered, B per requestManagedBuffered, B per requestioxide.nghttp3now allocates nothing per request.Fixes #487.