comms/uniflow/controller: take a span in sendAllVec (#3908) - #3908
Closed
cppccppccppc wants to merge 5 commits into
Closed
comms/uniflow/controller: take a span in sendAllVec (#3908)#3908cppccppccppc wants to merge 5 commits into
cppccppccppc wants to merge 5 commits into
Conversation
meta-cla
Bot
added
the
CLA Signed
Contributor
|
@cppccppccppc has exported this pull request. If you are a Meta employee, you can view the originating Diff in D117414473. |
cppccppccppc
pushed a commit
to cppccppccppc/torchcomms
that referenced
this pull request
Sep 1, 2026
Summary: Pull Request resolved: meta-pytorch#3908 sendAllVec took (iovec*, int) and tracked its position with a separate idx cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]` arithmetic. std::span carries the count with the pointer, which lets the cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the loop condition becomes !iov.empty(). That is the real win rather than the shorter signature. The arithmetic being deleted lives in the short-write branch, which the comment there notes is not reachable on a blocking socket except via a signal mid-transfer and is not covered by tests -- so it is the least safe place in the function to keep hand-rolled index math. It also drops the static_cast<size_t> on msg_iovlen, since size() is already size_t, and matches the idiom the rest of the interface uses: std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was already included and sendAllVec is private with one call site, so there is no ABI consideration. The call site's count becomes size_t and passes std::span{iov}.first(iovCnt), so the cast is removed rather than relocated to the caller. Kept the doc comment. A non-const std::span<iovec> conveys no more about mutation than a non-const iovec* did; what the comment carries is that the mutation is destructive bookkeeping -- the iov must not be reused after the call -- and no signature expresses that. Differential Revision: D117414473
cppccppccppc
force-pushed
the
export-D117414473
branch
from
September 1, 2026 07:05
e8a0054 to
bd41cbc
Compare
meta-codesync
Bot
changed the title
cppccppccppc
pushed a commit
to cppccppccppc/torchcomms
that referenced
this pull request
Sep 1, 2026
Summary: Pull Request resolved: meta-pytorch#3908 sendAllVec took (iovec*, int) and tracked its position with a separate idx cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]` arithmetic. std::span carries the count with the pointer, which lets the cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the loop condition becomes !iov.empty(). That is the real win rather than the shorter signature. The arithmetic being deleted lives in the short-write branch, which the comment there notes is not reachable on a blocking socket except via a signal mid-transfer and is not covered by tests -- so it is the least safe place in the function to keep hand-rolled index math. It also drops the static_cast<size_t> on msg_iovlen, since size() is already size_t, and matches the idiom the rest of the interface uses: std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was already included and sendAllVec is private with one call site, so there is no ABI consideration. The call site's count becomes size_t and passes std::span{iov}.first(iovCnt), so the cast is removed rather than relocated to the caller. Kept the doc comment. A non-const std::span<iovec> conveys no more about mutation than a non-const iovec* did; what the comment carries is that the mutation is destructive bookkeeping -- the iov must not be reused after the call -- and no signature expresses that. Differential Revision: D117414473
cppccppccppc
force-pushed
the
export-D117414473
branch
from
September 1, 2026 07:12
bd41cbc to
0c8398a
Compare
cppccppccppc
pushed a commit
to cppccppccppc/torchcomms
that referenced
this pull request
Sep 1, 2026
Summary: Pull Request resolved: meta-pytorch#3908 sendAllVec took (iovec*, int) and tracked its position with a separate idx cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]` arithmetic. std::span carries the count with the pointer, which lets the cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the loop condition becomes !iov.empty(). That is the real win rather than the shorter signature. The arithmetic being deleted lives in the short-write branch, which the comment there notes is not reachable on a blocking socket except via a signal mid-transfer and is not covered by tests -- so it is the least safe place in the function to keep hand-rolled index math. It also drops the static_cast<size_t> on msg_iovlen, since size() is already size_t, and matches the idiom the rest of the interface uses: std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was already included and sendAllVec is private with one call site, so there is no ABI consideration. The call site's count becomes size_t and passes std::span{iov}.first(iovCnt), so the cast is removed rather than relocated to the caller. Kept the doc comment. A non-const std::span<iovec> conveys no more about mutation than a non-const iovec* did; what the comment carries is that the mutation is destructive bookkeeping -- the iov must not be reused after the call -- and no signature expresses that. Differential Revision: D117414473
cppccppccppc
force-pushed
the
export-D117414473
branch
from
September 1, 2026 15:26
0c8398a to
24da0f0
Compare
cppccppccppc
pushed a commit
to cppccppccppc/torchcomms
that referenced
this pull request
Sep 1, 2026
Summary: Pull Request resolved: meta-pytorch#3908 sendAllVec took (iovec*, int) and tracked its position with a separate idx cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]` arithmetic. std::span carries the count with the pointer, which lets the cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the loop condition becomes !iov.empty(). That is the real win rather than the shorter signature. The arithmetic being deleted lives in the short-write branch, which the comment there notes is not reachable on a blocking socket except via a signal mid-transfer and is not covered by tests -- so it is the least safe place in the function to keep hand-rolled index math. It also drops the static_cast<size_t> on msg_iovlen, since size() is already size_t, and matches the idiom the rest of the interface uses: std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was already included and sendAllVec is private with one call site, so there is no ABI consideration. The call site's count becomes size_t and passes std::span{iov}.first(iovCnt), so the cast is removed rather than relocated to the caller. Kept the doc comment. A non-const std::span<iovec> conveys no more about mutation than a non-const iovec* did; what the comment carries is that the mutation is destructive bookkeeping -- the iov must not be reused after the call -- and no signature expresses that. Differential Revision: D117414473
cppccppccppc
force-pushed
the
export-D117414473
branch
from
September 1, 2026 15:32
24da0f0 to
8cbbda1
Compare
cppccppccppc
pushed a commit
to cppccppccppc/torchcomms
that referenced
this pull request
Sep 2, 2026
Summary: Pull Request resolved: meta-pytorch#3908 sendAllVec took (iovec*, int) and tracked its position with a separate idx cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]` arithmetic. std::span carries the count with the pointer, which lets the cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the loop condition becomes !iov.empty(). That is the real win rather than the shorter signature. The arithmetic being deleted lives in the short-write branch, which the comment there notes is not reachable on a blocking socket except via a signal mid-transfer and is not covered by tests -- so it is the least safe place in the function to keep hand-rolled index math. It also drops the static_cast<size_t> on msg_iovlen, since size() is already size_t, and matches the idiom the rest of the interface uses: std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was already included and sendAllVec is private with one call site, so there is no ABI consideration. The call site's count becomes size_t and passes std::span{iov}.first(iovCnt), so the cast is removed rather than relocated to the caller. Kept the doc comment. A non-const std::span<iovec> conveys no more about mutation than a non-const iovec* did; what the comment carries is that the mutation is destructive bookkeeping -- the iov must not be reused after the call -- and no signature expresses that. Differential Revision: D117414473
cppccppccppc
force-pushed
the
export-D117414473
branch
2 times, most recently
from
September 2, 2026 08:06
414e0f2 to
c1fa1b2
Compare
cppccppccppc
pushed a commit
to cppccppccppc/torchcomms
that referenced
this pull request
Sep 2, 2026
Summary: Pull Request resolved: meta-pytorch#3908 sendAllVec took (iovec*, int) and tracked its position with a separate idx cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]` arithmetic. std::span carries the count with the pointer, which lets the cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the loop condition becomes !iov.empty(). That is the real win rather than the shorter signature. The arithmetic being deleted lives in the short-write branch, which the comment there notes is not reachable on a blocking socket except via a signal mid-transfer and is not covered by tests -- so it is the least safe place in the function to keep hand-rolled index math. It also drops the static_cast<size_t> on msg_iovlen, since size() is already size_t, and matches the idiom the rest of the interface uses: std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was already included and sendAllVec is private with one call site, so there is no ABI consideration. The call site's count becomes size_t and passes std::span{iov}.first(iovCnt), so the cast is removed rather than relocated to the caller. Kept the doc comment. A non-const std::span<iovec> conveys no more about mutation than a non-const iovec* did; what the comment carries is that the mutation is destructive bookkeeping -- the iov must not be reused after the call -- and no signature expresses that. Differential Revision: D117414473
cppccppccppc
pushed a commit
to cppccppccppc/torchcomms
that referenced
this pull request
Sep 2, 2026
Summary: Pull Request resolved: meta-pytorch#3908 sendAllVec took (iovec*, int) and tracked its position with a separate idx cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]` arithmetic. std::span carries the count with the pointer, which lets the cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the loop condition becomes !iov.empty(). That is the real win rather than the shorter signature. The arithmetic being deleted lives in the short-write branch, which the comment there notes is not reachable on a blocking socket except via a signal mid-transfer and is not covered by tests -- so it is the least safe place in the function to keep hand-rolled index math. It also drops the static_cast<size_t> on msg_iovlen, since size() is already size_t, and matches the idiom the rest of the interface uses: std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was already included and sendAllVec is private with one call site, so there is no ABI consideration. The call site's count becomes size_t and passes std::span{iov}.first(iovCnt), so the cast is removed rather than relocated to the caller. Kept the doc comment. A non-const std::span<iovec> conveys no more about mutation than a non-const iovec* did; what the comment carries is that the mutation is destructive bookkeeping -- the iov must not be reused after the call -- and no signature expresses that. Differential Revision: D117414473
cppccppccppc
force-pushed
the
export-D117414473
branch
from
September 2, 2026 08:12
c1fa1b2 to
fa77193
Compare
cppccppccppc
pushed a commit
to cppccppccppc/torchcomms
that referenced
this pull request
Sep 2, 2026
Summary: Pull Request resolved: meta-pytorch#3908 sendAllVec took (iovec*, int) and tracked its position with a separate idx cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]` arithmetic. std::span carries the count with the pointer, which lets the cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the loop condition becomes !iov.empty(). That is the real win rather than the shorter signature. The arithmetic being deleted lives in the short-write branch, which the comment there notes is not reachable on a blocking socket except via a signal mid-transfer and is not covered by tests -- so it is the least safe place in the function to keep hand-rolled index math. It also drops the static_cast<size_t> on msg_iovlen, since size() is already size_t, and matches the idiom the rest of the interface uses: std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was already included and sendAllVec is private with one call site, so there is no ABI consideration. The call site's count becomes size_t and passes std::span{iov}.first(iovCnt), so the cast is removed rather than relocated to the caller. Kept the doc comment. A non-const std::span<iovec> conveys no more about mutation than a non-const iovec* did; what the comment carries is that the mutation is destructive bookkeeping -- the iov must not be reused after the call -- and no signature expresses that. Differential Revision: D117414473
cppccppccppc
force-pushed
the
export-D117414473
branch
2 times, most recently
from
September 2, 2026 23:46
6cbd60c to
17d1d75
Compare
cppccppccppc
pushed a commit
to cppccppccppc/torchcomms
that referenced
this pull request
Sep 2, 2026
Summary:
sendAllVec took (iovec*, int) and tracked its position with a separate idx
cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]`
arithmetic. std::span carries the count with the pointer, which lets the
cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the
loop condition becomes !iov.empty().
That is the real win rather than the shorter signature. The arithmetic being
deleted lives in the short-write branch, which the comment there notes is not
reachable on a blocking socket except via a signal mid-transfer and is not
covered by tests -- so it is the least safe place in the function to keep
hand-rolled index math.
It also drops the static_cast<size_t> on msg_iovlen, since size() is already
size_t, and matches the idiom the rest of the interface uses:
std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was
already included and sendAllVec is private with one call site, so there is no
ABI consideration.
The call site's count becomes size_t and passes std::span{iov}.first(iovCnt),
so the cast is removed rather than relocated to the caller.
Kept the doc comment. A non-const std::span<iovec> conveys no more about
mutation than a non-const iovec* did; what the comment carries is that the
mutation is destructive bookkeeping -- the iov must not be reused after the
call -- and no signature expresses that.
Differential Revision: D117414473
cppccppccppc
pushed a commit
to cppccppccppc/torchcomms
that referenced
this pull request
Sep 2, 2026
Summary:
sendAllVec took (iovec*, int) and tracked its position with a separate idx
cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]`
arithmetic. std::span carries the count with the pointer, which lets the
cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the
loop condition becomes !iov.empty().
That is the real win rather than the shorter signature. The arithmetic being
deleted lives in the short-write branch, which the comment there notes is not
reachable on a blocking socket except via a signal mid-transfer and is not
covered by tests -- so it is the least safe place in the function to keep
hand-rolled index math.
It also drops the static_cast<size_t> on msg_iovlen, since size() is already
size_t, and matches the idiom the rest of the interface uses:
std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was
already included and sendAllVec is private with one call site, so there is no
ABI consideration.
The call site's count becomes size_t and passes std::span{iov}.first(iovCnt),
so the cast is removed rather than relocated to the caller.
Kept the doc comment. A non-const std::span<iovec> conveys no more about
mutation than a non-const iovec* did; what the comment carries is that the
mutation is destructive bookkeeping -- the iov must not be reused after the
call -- and no signature expresses that.
Differential Revision: D117414473
added 5 commits
September 2, 2026 17:58
…rrected numbers (meta-pytorch#3897) Summary: --tcp-sockbuf exposes the data connection's SO_SNDBUF/SO_RCVBUF so the value can be swept instead of argued about, and the bench brackets each size's timed loop with TcpTransport::logAndResetPhaseStats() so every bandwidth line is accompanied by where the time went. The script header carried get ~1.0 GB/s, which predated the pinned-staging work and was stale by roughly 9x. It misled a full round of planning, so it is corrected here to measured values along with the link baseline (200G, MTU 1500, RTT 0.046 ms, iperf3 15.4 GB/s single stream and 23.3 GB/s over 8) that makes the numbers interpretable. Recipe 8 records a disproof rather than a hypothesis. The theory was that the 1 MiB buffer pin caps a stream at window/RTT; at 0.046 ms RTT, 1 MiB permits ~22.8 GB/s, so the window was never the constraint. A 6-point sweep with 3 repeats does show the 1 MiB pin is the worst non-degenerate setting (8.63 GB/s at 1 GiB against 9.67 unpinned, disjoint ranges), but the mechanism is autotuning being disabled while the reader does a multi-MiB copy, not the bandwidth-delay product. The 64K arm is a deliberate control: it drops throughput 75%, which is what makes the flat region above 1 MiB trustworthy rather than merely consistent with a dead knob. Also records that a --no-verify arm must never be compared against a verifying one -- that mistake inverted this exact comparison once. Reviewed By: yexiangd Differential Revision: D117132684
…ytorch#3913) Summary: Phase 3d makes slab-backed VRAM ReadReply copies asynchronous on the caller stream. The transport retains the receive slab and destination write reservation until CUDA event completion, polls all pending events from the EventBase so later copies can retire out of order, and drains or quarantines copies safely across query errors and shutdown. Vector-backed fallback remains synchronous because its source storage is reused immediately. The benchmark exposes --no-tcp-async-h2d for controlled comparisons and reports the active mode. # A failed completion probe is not a failed copy Two paths conflated "I could not observe this copy finish" with "this copy did not finish", and reported a transfer error for data that had demonstrably landed. In `stageAsyncH2d`, a failing `eventRecord` arrives after `memcpyAsync` has already succeeded, so the copy is in flight and only the tracking mechanism is gone. The code falls back to `waitForH2dCopy`, and if that wait succeeds the copy has completed and the destination holds the payload -- but the old code then returned the `eventRecord` error, failing the caller's get for a copy that worked. It now completes with `Ok()`. `pollPendingH2d` had the same shape: a failing `eventQuery` left its error in `result`, and a successful `waitForH2dCopy` fell through without clearing it, retiring a completed copy as failed. The recovery now sets `result = Ok()` before retirement. The sibling site at the end of `stageAsyncH2d` deliberately keeps propagating `status`: there the error is the transport stopping or the pending record failing to track, which is a real operation failure even though the synchronize proves the copy finished. Only the two probe-failure paths change. Reviewed By: yexiangd Differential Revision: D117155852
…eta-pytorch#3918) Summary: TcpAsyncAcceptTest derived the address family by comparing the param's clientHost against the "127.0.0.1" literal. That defaults every other spelling -- "localhost", "[::1]", a resolvable hostname -- to AF_INET6 without saying so, and the two socket-buffer tests feed that family to kernelDefaultRcvBuf/rcvBufForRequest. A param added later would probe the wrong family and surface as a confusing skip or a wrong expectation rather than a clear failure. Store the family on AddrFamily and state it at the two INSTANTIATE_TEST_SUITE_P entries, so a new param has to declare which family it is. A helper function would have deduplicated the comparison but kept the default. The five sites this replaces are the two family derivations in the socket-buffer tests, the socket()/sockaddr pair in AsyncAcceptRejectsNonUniflowClient, and the test-name lambda -- which now derives the name from the same field the bodies use, so the two cannot disagree. AddrFamily is defined separately in four test files; this changes only TcpAsyncAcceptTest.cpp. The same pattern remains in TcpConnTest.cpp:48 (inverse polarity) and the other three name lambdas. Reviewed By: yexiangd Differential Revision: D117407765
… sockets (meta-pytorch#3904) Summary: D117132557 threaded TcpSocketConfig into the accept path but configureAcceptedSocket consumed only socketBufSize and hardcoded the other seven fields. That is a worse shape than the old `int acceptRetryCnt` Reviewed By: yexiangd Differential Revision: D117409938
Summary:
sendAllVec took (iovec*, int) and tracked its position with a separate idx
cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]`
arithmetic. std::span carries the count with the pointer, which lets the
cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the
loop condition becomes !iov.empty().
That is the real win rather than the shorter signature. The arithmetic being
deleted lives in the short-write branch, which the comment there notes is not
reachable on a blocking socket except via a signal mid-transfer and is not
covered by tests -- so it is the least safe place in the function to keep
hand-rolled index math.
It also drops the static_cast<size_t> on msg_iovlen, since size() is already
size_t, and matches the idiom the rest of the interface uses:
std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was
already included and sendAllVec is private with one call site, so there is no
ABI consideration.
The call site's count becomes size_t and passes std::span{iov}.first(iovCnt),
so the cast is removed rather than relocated to the caller.
Kept the doc comment. A non-const std::span<iovec> conveys no more about
mutation than a non-const iovec* did; what the comment carries is that the
mutation is destructive bookkeeping -- the iov must not be reused after the
call -- and no signature expresses that.
Reviewed By: yexiangd
Differential Revision: D117414473
cppccppccppc
force-pushed
the
export-D117414473
branch
from
September 3, 2026 00:59
17d1d75 to
5246e41
Compare
Contributor
|
This pull request has been merged in a44e7e7. |
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
1 participant
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:
sendAllVec took (iovec*, int) and tracked its position with a separate idx
cursor, so the loop carried
iov + idx,iovCnt - idxandiov[idx]arithmetic. std::span carries the count with the pointer, which lets the
cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the
loop condition becomes !iov.empty().
That is the real win rather than the shorter signature. The arithmetic being
deleted lives in the short-write branch, which the comment there notes is not
reachable on a blocking socket except via a signal mid-transfer and is not
covered by tests -- so it is the least safe place in the function to keep
hand-rolled index math.
It also drops the static_cast<size_t> on msg_iovlen, since size() is already
size_t, and matches the idiom the rest of the interface uses:
std::span on send, std::span<uint8_t> on recv. was
already included and sendAllVec is private with one call site, so there is no
ABI consideration.
The call site's count becomes size_t and passes std::span{iov}.first(iovCnt),
so the cast is removed rather than relocated to the caller.
Kept the doc comment. A non-const std::span conveys no more about
mutation than a non-const iovec* did; what the comment carries is that the
mutation is destructive bookkeeping -- the iov must not be reused after the
call -- and no signature expresses that.
Reviewed By: yexiangd
Differential Revision: D117414473