perf(transfer)!: run-encode QueryBlocksForTransfer addresses and coalesce RDMA READs#350
Open
xiaguan wants to merge 1 commit into
Open
perf(transfer)!: run-encode QueryBlocksForTransfer addresses and coalesce RDMA READs#350xiaguan wants to merge 1 commit into
xiaguan wants to merge 1 commit into
Conversation
…esce RDMA READs Replace the per-segment fixed64 address list with per-lane affine runs (base + i * stride): holder memory comes from sequential bump allocation, so a lane normally collapses to a single run and the query response drops from ~4.6 MB to a few KB for a 1000-block prefix. The requester decodes the runs, groups runs whose per-block chunks tile densely, mirrors each group into a dense local slab region, and issues one RDMA READ per dense group instead of one per segment. Fragmented holder layouts degrade gracefully to per-block or per-segment reads with the same arithmetic rebuild path. BREAKING CHANGE: QueryBlocksForTransferResponse replaces segment_ptrs with runs_per_lane/run_base/run_stride/run_count; older peers cannot interoperate.
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.
Follow-up to #349 (p2p_bench example), rebased onto master after its merge. Single commit.
Problem
#349's benchmark showed the v1 cross-node fetch path is bookkeeping-bound, not bandwidth-bound. For the default shape (1000 blocks × 288 slots × 2 segments = 576k segments of 4 KiB, 2250 MiB per set, two 8×H200 nodes with 8×400G IB):
fixed64per segment → ~4.6 MB on the wireTransferDesc+ 288k smallVecsSegmentobjectsEvery stage is O(segments). The fix exploits one structural fact: holder segment addresses come from sequential bump allocation, so per lane (= one slot's K or V segment across the block prefix) they form an arithmetic sequence
addr(i) = base + i × stride— and within a block, all slots on the same NUMA node are contiguous.What this PR does
Wire format (
engine.proto):QueryBlocksForTransferResponsereplacesrepeated fixed64 segment_ptrs(one per segment) with run-encoded lanes —runs_per_lane/run_base/run_stride/run_countparallel arrays. A fully bump-allocated holder collapses every lane to a single run: the response drops from ~4.6 MB to ~13 KB. Worst case (every lane fragmented per block) is ~8.6 MB, comparable to the old format.segment_ptrsonly ever existed on this unmerged PR stack, so there is no compatibility concern; the field is replaced, not reserved.Holder (
pegaflow-core/src/lib.rs): the prefix scan now verifies each block against the slot template first, then feeds segment addresses into per-lane greedy run accumulators (RunTableBuilder). Two-phase per block, so a mid-block layout mismatch never leaks partial addresses into the table.Requester (
pegaflow-core/src/transfer_runs.rs::plan_transfer, new module shared by both sides):block_count, no zero-size K slots, remote spans don't overflow).SealedBlocks arithmetically:local(b) = group_local + (remote(b) − group_remote), every pointer bounds-checked throughNumaSlab::ptr_at.READ splitting (
rdma_fetch.rs): planned descs are split atMAX_RDMA_READ_BYTES = 32 MiB. Two reasons: a single WQE must stay under the HCA max message size (1 GiB on mlx5 — the first unsplit run died withIBV_WC_LOC_LEN_ERRon a 1125 MiB READ), and multiple descriptors let the engine stripe a dense region across QPs/NICs.Trust boundary
The run table and slot template are remote input. The planner rejects inconsistent tables (wrong lane count, bad coverage, zero-count runs, zero-size K slots, span overflow); all local offset arithmetic is checked, and every materialized pointer goes through slab bounds checks. A corrupt-but-consistent table can at worst fill our own buffers with garbage — it cannot produce overlapping or out-of-bounds local writes, and it cannot construct a zero-segment block (deferred-panic vector found in review and closed).
Results
Same two-node setup as #349,
--verify(every byte, all 8 ranks) passing on all runs.Stage breakdown after (4 KiB shape, median): query 6–29 ms, build 0.17 ms, submit 0.10 ms, rdma_wait 11–21 ms, rebuild ~35 ms. Rebuild is now the dominant stage (576k
Segmentobject constructions) — that is the next PR (seal from template metadata), kept out of this one deliberately.Testing
transfer_runs.rs; the plan tests all run anassert_plan_replayscheck that re-derives every (block, lane) address through both the desc copy and the run arithmetic and asserts they agree — i.e. they test the actual correctness property, not implementation details.cargo test -p pegaflow-core --lib: 117 passed.Review guide
pegaflow-core/src/transfer_runs.rsis the heart (~680 lines incl. tests):RunTableBuilder(encode),plan_transfer(validate/group/layout),group_accepts(the dense-tiling merge condition — the one function where a bug corrupts data).rdma_fetch.rs::fetch_blocks_via_rdma+rebuild_sealed_blocks: plan execution, 32 MiB split, cursor walk.lib.rs::query_blocks_for_transfer: two-phase scan feeding the builder.service.rs: pure plumbing.BREAKING CHANGE:
QueryBlocksForTransferResponsereplacessegment_ptrswithruns_per_lane/run_base/run_stride/run_count; older peers cannot interoperate.🤖 Generated with Claude Code