Repository navigation
Conversation
SharedVector could only own a whole vector and view all of it. SharedSpan
separates what keeps the bytes alive from what is visible:
std::shared_ptr<const void> _owner; // may be null
Span<const T> _view;
shared_ptr<U> converts to shared_ptr<const void> while keeping U's deleter in
the control block, so the owner can be type erased and a payload can alias a
slice of a larger buffer. One type now covers an owning copy, a view into a
buffer owned elsewhere, and a borrowed view.
Send path:
The copy of the user's payload at the wire conversion stays and is the only
one. It is mandatory: VAsioConnection::SendMsg defers to the I/O thread via
asio::post, so the caller's Span is already dangling by the time serialization
runs. Everything after that copy is refcounted.
VAsioTransmitter no longer re-serializes once per remote peer. The serialized
form differs per receiver only in the remoteIndex field of the network header,
so the message is serialized once; each peer copies the 29 byte header, patches
its own remoteIndex, and writes header plus shared body as a two buffer gather
write. The send queue holds the body by reference count, so it outlives the
slowest peer, including across a disconnect mid-write.
Receive path:
The copy out of the ring buffer stays, since the ring wraps and a message may
be physically discontiguous, but it now lands in a shared_ptr owned blob, one
per message. Deserialized byte payloads alias that blob via a range checked
Subspan instead of being copied out of it again. The blob is anchored by the
MsgT local in VAsioReceiver::ReceiveRawMsg for the whole handler dispatch.
Also:
- ExecuteOnIoThread captured its arguments by copy, silently discarding the
move that the rvalue SendMsg overloads perform. It now moves them.
- MessageBuffer no longer zero fills bytes it immediately overwrites, and the
stale static SerializedSize hint is replaced by a per message estimate.
- The per received message 4 byte heap peek is now a stack array, which also
fixes a misaligned read through reinterpret_cast.
- RemoteServiceEndpoint references the descriptor instead of deep copying it.
- RPC wire types move off plain std::vector, dropping the ToStdVector copies.
Public API, ABI and wire format are unchanged; SharedSpan does not appear in
any installed header and no protocol version bump is needed. The Ethernet runt
padding buffer is deliberately left as is: it is correct under the documented
callback contract, and changing it only matters for a retained payload API that
is not being added.
Received payloads must not reach MessageHistory, which retains for the
participant's lifetime. That holds structurally today, since VAsioTransmitter is
only reachable from DistributeLocalSilKitMessage; it is documented at both ends
with SharedSpan::Cloned() as the escape hatch.
Signed-off-by: Marius Börschig <Marius.Boerschig@vector.com>
…opies Signed-off-by: Marius Börschig <Marius.Boerschig@vector.com>
Signed-off-by: Marius Börschig <Marius.Boerschig@vector.com>
…ference Signed-off-by: Marius Börschig <Marius.Boerschig@vector.com>
…inst reference style conventions Signed-off-by: Marius Börschig <Marius.Boerschig@vector.com>
Signed-off-by: Marius Börschig <Marius.Boerschig@vector.com>
Collaborator
Author
|
the performance improvements for big messages / many receivers look good. for a single remote receiver there is a slight regression, because we have an extra allocation. TODO adding a fast path for single peer, and inlining small messages not just headers |
Signed-off-by: Marius Börschig <Marius.Boerschig@vector.com>
Signed-off-by: Marius Börschig <Marius.Boerschig@vector.com>
Collaborator
Author
|
@VDanielEdwards i changed the benchmark tool to provide output to my liking (and reusing build dir), we can easily revert those independent of the improvements in the code here. Did some benchmarks, on my (very loaded) windows11 workstation and WSL (i9-13950HX, 128GB): WSL on same machine: on a bare metal Ubuntu 24.04.4 (i9-13900K, 64GB): tweaking the inline-message-size from 128 to 256 gives some improvements on WSL, but nothing justifiable. ==> i think this is still valuable. the high variance of test results on my machine may be just noise. |
MariusBgm
marked this pull request as ready for review
September 15, 2026 14:11
MariusBgm
force-pushed
the
dev/zero_copy
branch
from
September 16, 2026 08:59
be452ec to
3e2f05a
Compare
MariusBgm
added
the
status: ready for review
Signed-off-by: Marius Börschig <Marius.Boerschig@vector.com>
snps-fricke
approved these changes
Oct 7, 2026
snps-fricke
left a comment
snps-fricke
left a comment
Collaborator
There was a problem hiding this comment.
This is a very nice idea and a sleek solution!
As for the risk assessment you asked for, I think let's trust in the test coverage. The performance improvement is definitely worth it.
| * MakeSharedSpan(), which range-check. | ||
| */ | ||
| template <typename U> | ||
| SharedSpan(std::shared_ptr<U> owner, Span<const T> view); |
Collaborator
There was a problem hiding this comment.
This allows creating invalid or out-of-bound instances. You already discourage its use and i only saw it being used by MakeSharedSpan and the test.
Can we enforce using the safer option? How about making this private and MakeSharedSpan a static method?
| SerializedMessage message{std::move(currentMsg)}; | ||
| const auto blobSize = currentMsg->size(); | ||
| SerializedMessage message{ | ||
| SilKit::Util::MakeSharedSpan(std::shared_ptr<const std::vector<uint8_t>>{std::move(currentMsg)}, 0, |
Collaborator
There was a problem hiding this comment.
Apart from tests, this seems to be the only call site of MakeSharedSpan.
The offset is 0 here and the count is the entire blob size. Can we make use of the SharedSpan(std::vector) instead? I think MakeSharedSpan as well as the constructor it uses could be removed, then.
I think that would give some clarity, as then Subspan() is the only way to create the "narrowed down" version.
snps-fiodorov
removed
the
status: ready for review
KonradBreitsprecherBkd
added
the
status: approved
- Moving a SharedSpan now empties the source, which previously kept a view into storage it no longer owned. - VAsioPeer now moves the received vector into a SharedSpan, which leaves MakeSharedSpan and the unchecked (owner, view) constructor with no callers, so both are removed. Subspan() is now the only way to narrow a view. - Reword the constructor comment: a vector passed as an rvalue is moved, everything else is copied. Signed-off-by: Marius Börschig <marius.boerschig@vector.com>
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.
SharedVector could only own a whole vector and view all of it. SharedSpan separates what keeps the bytes alive from what is visible:
shared_ptr converts to shared_ptr while keeping U's deleter in the control block, so the owner can be type erased and a payload can alias a slice of a larger buffer. One type now covers an owning copy, a view into a buffer owned elsewhere, and a borrowed view.
Send path:
The copy of the user's payload at the wire conversion stays and is the only one. It is mandatory: VAsioConnection::SendMsg defers to the I/O thread via asio::post, so the caller's Span is already dangling by the time serialization runs. Everything after that copy is refcounted.
VAsioTransmitter no longer re-serializes once per remote peer. The serialized form differs per receiver only in the remoteIndex field of the network header, so the message is serialized once; each peer copies the 29 byte header, patches its own remoteIndex, and writes header plus shared body as a two buffer gather write. The send queue holds the body by reference count, so it outlives the slowest peer, including across a disconnect mid-write.
Receive path:
The copy out of the ring buffer stays, since the ring wraps and a message may be physically discontiguous, but it now lands in a shared_ptr owned blob, one per message. Deserialized byte payloads alias that blob via a range checked Subspan instead of being copied out of it again. The blob is anchored by the MsgT local in VAsioReceiver::ReceiveRawMsg for the whole handler dispatch.
Also:
Public API, ABI and wire format are unchanged; SharedSpan does not appear in any installed header and no protocol version bump is needed. The Ethernet runt padding buffer is deliberately left as is: it is correct under the documented callback contract, and changing it only matters for a retained payload API that is not being added.
Received payloads must not reach MessageHistory, which retains for the participant's lifetime. That holds structurally today, since VAsioTransmitter is only reachable from DistributeLocalSilKitMessage; it is documented at both ends with SharedSpan::Cloned() as the escape hatch.