Conversation
|
@seanmonstar any update on when this might be merged in? |
Member
|
I was distracted with a separate large refactor, but I'm less certain the big one will work. I'm turning attention to smaller performance wins. (Sorry about the merge conflicts.) I think this could be a good improvement! Originally we were concerned about being able to quickly write control frames when needed, but I think this should be fine in practice. Especially if after a batch is started and a partial write happens, a follow-up only needs to finish the current partial DATA frame, and then could allow control frames through again. |
8 of 9 tasks
4 tasks
wkirschenmann
added a commit
to wkirschenmann/ArmoniK.Api
that referenced
this pull request
Oct 5, 2026
… h2 patch packages/rust/patches/h2-batch holds a patch of h2 0.4.19 and the recipe that builds against it; nothing else of h2 is in the repository. The patch is the POC branch's: hyperium/h2#903 ported onto 0.4.19, several DATA frames queued and written in one vectored write, and a queued part spanning several frames of the peer's largest size. The count of frames per part is no longer the process's AK_H2_COALESCE: it is the connection's, set by `h2::with_frames_per_write(frames, future)` for every codec made while the future is polled, 1 outside it; the value a poll found is put back even when it panics, and the frame-length products saturate. It also corrects hyperium/h2#903: a stream reset by the peer while one of its parts is half written, and released by the time the part is, made the connection panic on "dangling store key" when the written part came back to be reclaimed, at any count of frames. The rest of the part is now dropped when its stream is gone (`Store::contains`). build.sh reads h2's version and checksum in Cargo.lock, CRLF or not, and refuses any h2 but 0.4.19, takes the crate from cargo's cache (fetched there first if missing), checks its sha256, extracts and patches it into target/h2-batch, and runs cargo with `--config patch.crates-io.h2.path`, after the subcommand so that clippy gets it too, and ARMONIK_H2_BATCH=1, for that run only, putting back the Cargo.lock a path patch rewrites. Runs on one target directory take turns, a directory made and removed around the whole run, since each rewrites the same copy and the same Cargo.lock and the .NET binding builds once per target framework in parallel; a turn still held after 30 minutes fails the run. The copy is extracted again only when the crate or the patch changed, which spares rebuilding h2 and all above it. A `+toolchain` first argument goes before the subcommand. Under Git Bash on Windows it gives cargo a Windows path and keeps Git's link.exe off the linker's path. The .NET binding builds its engine that way when NativeEngineH2Batch is true, and its error then names the bash it needs. A CI job, test-rust-h2-batch, runs the engine's tests through the recipe, so a patch that no longer applies, or an h2 other than 0.4.19 in Cargo.lock, fails there. The patch directory keeps its line endings and trailing spaces through git and editors. armonik-transport's new build script turns ARMONIK_H2_BATCH=1 into cfg(h2_batch), under which the handshake runs inside with_frames_per_write. The option is Http2.Send.FramesPerWrite, 1 to 256, 1 by default, the schema's maximum taken from LARGEST_FRAMES_PER_WRITE; above 1 it is refused by a build without the patch, at the option and in Http2Config, both naming the patch's directory. Above 1, a reset call may send up to that many frames less one before its RST_STREAM. At any count the patched build differs from stock h2: a control frame waits behind the DATA already queued, up to one part per active stream, where stock h2 holds one frame. Verified on Windows x64: - cargo test -p armonik-transport -p armonik-transport-ffi --all-features, stock h2: every suite ok, with frames_per_write_need_the_patched_build, the refusals of 0 and 257 in a_setting_no_session_could_use_is_refused, and the option's own in the options tests. - The same through `patches/h2-batch/build.sh test ...`, h2 built from the patched copy: every suite ok. The two tests of tests/h2_batch.rs take turns, since the write count they read is the process's. more_frames_per_write_is_fewer_writes: a 4 MiB request over loopback goes out in 17 writes at 16 frames, 257 at 1, and its echo, bytes counting up, comes back equal. a_request_reset_mid_write_leaves_the_connection_usable: a hand-written server stops reading at the first DATA frame of a 32 MiB request, resets it once the client's writes block, resumes reading, and answers the next call on the same connection; at 1 and at 16 frames that call is OK, 20 runs out of 20. Against the patch without the `Store::contains` check it fails 10 runs out of 10, the connection task panicking on "dangling store key for stream_id=StreamId(1)". Cargo.lock comes back unchanged. - Two runs of build.sh started together: the second waits for the first, both succeed, Cargo.lock is unchanged and the turn is released. A run on an unchanged copy rebuilds nothing. - cargo clippy, stock and through the recipe: the libraries with -Dwarnings -Dunused-crate-dependencies, and armonik-transport's --tests with -Dwarnings, clean. - cargo fmt --all --check: clean. - Schemas and generated C# written again from their tools. - RustGrpcChannel tests on net4.7, net4.8, net8.0, net10.0 and net11.0, and the options generator's tests: all passed.
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
3 participants
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
This PR addresses the sender-side perf issue described in #902
by allowing multiple DATA frames to be coalesced into a single write_vectored() syscall,
rather than the current behavior of flushing each DATA frame individually.
Problem
The existing encoder holds at most one pending DATA frame at a time and blocks accepting new frames until it is fully flushed. With TCP_NODELAY enabled, each frame triggers its own write() syscall and TCP segment — inflating the syscall-to-payload ratio and cutting throughput roughly in half compared to raw TCP.
Solution
Option<Next<B>>slot with aVecDeque<BufElement<B>>that can queue multiple DATA frames (up to 512 on vectored-IO transports).Bufdirectly on the Encoder, providingchunks_vectored()so that all queued frame headers + payloads are written in onepoll_write_vectored()call.in_flight_data_framefield with a per-streamin_flight_partial_sendfield of typeOption<ControlFlow<()>>, allowing each stream to send up to one partial data frame at a time without blocking other streams.take_used_data_frames()iterator to reclaim all fully-written frames in a single pass, replacing the oldtake_last_data_frame()which could only return one.poll_write_bufthat returnsControlFlowto cleanly distinguish "nothing left to write" from "wrote some bytes, keep going".Validation
Perf results for a branch including the commit in this PR:
Receiver-side PR: #904