Reuse a per-connection encode buffer for daemon replies - #3157
Closed
phil-opp wants to merge 1 commit into
Closed
Conversation
`TcpConnection::send_reply` allocated a fresh `Vec` per reply. It
already takes `&mut self`, so the buffer can live on the connection:
`dora_message::encode_into` takes a buffer and hands it back.
Measured, rather than estimated -- and it is smaller than the estimate
that motivated the idea (30-100 ns):
payload fresh reused saved share of encode
0 B 59.5ns 51.4ns 8.0ns 14%
64 B 67.7ns 56.2ns 11.5ns 17%
4 KB 100.8ns 86.1ns 14.7ns 15%
~15% of encode cost, but encode is a small part of `send_reply`, which
then awaits a socket write measured in microseconds -- so under 1% of
the operation. This is arguably below the bar for the state it adds;
the PR body says so explicitly so the call can be made on the numbers.
The retained buffer is capped at MAX_RETAINED_SEND_BUF (256 KiB).
Without that a single outsized reply would pin its full size -- up to
MAX_MESSAGE_BYTES, 64 MiB -- for the life of the connection, and the
daemon holds one connection per node. The buffer is also returned on
the send-failure path, so a transient socket error does not silently
drop the reuse for the rest of the connection.
`encode_into_ignores_the_buffers_previous_contents` covers the failure
mode that would actually hurt: a missing `clear()` leaving stale bytes
in a frame. It feeds in buffers that are empty, shorter, exactly equal,
4x longer, and large-but-empty-with-capacity, and asserts the output is
byte-identical to a fresh `encode` and still decodes.
Scoped to the daemon reply path, where `&mut self` already exists. The
node side is a free function over `&mut TcpStream`, so its buffer would
have to move onto the caller's connection struct -- more churn than the
measured win justifies.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Stacked on #<FOLLOWUP1_PR> (
followup/datamessage-serialize-bytes), which is itself stacked on the postcard migration. Review those first.Read the measurement before the diff — you may want to close this instead of merging it.
TcpConnection::send_replyallocated a freshVecper reply. Since it already takes&mut self, the buffer can live on the connection:dora_message::encode_intotakes a buffer and returns it, so the connection hands the same one back each time.What it's actually worth
I measured this rather than trusting the estimate in the issue, which guessed 30–100 ns. It's less than half the bottom of that range:
So: ~15% of encode cost, but encode is a small part of
send_reply, which then does an async socket write measured in microseconds. In context this is well under 1% of the operation.My read: this is below the bar for the state it adds, and I'd understand closing it. It's in your hands rather than mine because you asked for the PR — I'm giving you the numbers to decide on, not a recommendation to merge. If you do close it,
dora_message::encode_intoand its test are the only pieces I'd suggest keeping, and only if something else wants them.If it does land
The retained buffer is capped at
MAX_RETAINED_SEND_BUF(256 KiB). Without that, a single outsized reply would pin its full size — up toMAX_MESSAGE_BYTES, 64 MiB — for the life of the connection, and the daemon holds one connection per node. That's the one way this change could do real harm, so it's bounded explicitly rather than left toVec's growth policy.The buffer is also returned to the connection on the send-failure path, so a transient socket error doesn't silently drop the reuse for the rest of the connection's life.
encode_into_ignores_the_buffers_previous_contentscovers the failure mode that would actually hurt: a missingclear()leaving stale bytes in a frame. It feeds in buffers that are empty, shorter, exactly equal, 4x longer, and large-but-empty-with-capacity, and asserts the output is byte-identical to a freshencodeand still decodes.Scope
Only the daemon reply path, where
&mut selfalready exists. The node side (apis/rust/node/src/daemon_connection/tcp.rs::send_message) is a free function over&mut TcpStream, so the buffer would have to move onto the caller's connection struct — more churn than the measured win justifies, so I left it.Verification
cargo fmt --all -- --check,cargo clippy --all -- -D warnings— cleancargo test --all— 125 test binaries, zero failuresDaemonRequest::SendMessage, fresh-allocation vs held-buffer, same machine