perf(datapath): zero-copy proto layout and forwarding path improvements - #1762
perf(datapath): zero-copy proto layout and forwarding path improvements#1762Tehsmash wants to merge 19 commits into
Conversation
206aa30 to
c7d7e35
Compare
92b9810 to
6c8d9b2
Compare
|
The latest Buf updates on your PR. Results from workflow ci-buf / buf (pull_request).
|
c7d7e35 to
c37945e
Compare
dd8fffb to
1551cef
Compare
8b983ef to
4a9bc95
Compare
5f621d0 to
2577bc5
Compare
16c26ef to
83c727f
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| TransportChannel::Grpc(grpc_channel) => { | ||
| let mut client = DataPlaneServiceClient::new(grpc_channel); | ||
| let (tx, rx) = mpsc::channel(128); | ||
| let (tx, rx) = mpsc::channel(1024); |
There was a problem hiding this comment.
Is this the right value or a left over from the tests?
| .unwrap_or(NameId::NULL_COMPONENT), | ||
| .and_then(|s| uuid::Uuid::parse_str(s).ok()) | ||
| .map(|u| u.as_u128()) | ||
| .unwrap_or(NULL_COMPONENT), |
There was a problem hiding this comment.
Here if the id is set to "DATA_CHANNEL_ID" I think uuid::Uuid::parse_str(s) silently falling back to NULL_COMPONENT and causing incorrect routing. Use id_from_str() should solve the problem
| .unwrap_or(NameId::NULL_COMPONENT), | ||
| .and_then(|s| uuid::Uuid::parse_str(s).ok()) | ||
| .map(|u| u.as_u128()) | ||
| .unwrap_or(NULL_COMPONENT), |
There was a problem hiding this comment.
Here there is the same problem as in reconciler.rs
| .unwrap_or(NameId::NULL_COMPONENT), | ||
| .and_then(|s| uuid::Uuid::parse_str(s).ok()) | ||
| .map(|u| u.as_u128()) | ||
| .unwrap_or(NULL_COMPONENT), |
| return None; | ||
| } | ||
| let data = b.as_ref(); | ||
| let len0 = u32::from_le_bytes(data.get(0..4)?.try_into().unwrap()) as usize; |
There was a problem hiding this comment.
here if len < 4 this will panic. same for all the other steps.
d9320e9 to
4a43cf6
Compare
Enable TCP_NODELAY on outbound gRPC connections to disable Nagle's algorithm and reduce latency on small messages. Configure HTTP/2 flow control windows on both client and server (default 4 MiB per-stream, 16 MiB per-connection) to avoid hitting the 64 KB h2 default under high-throughput or high-RTT conditions. Increase the outbound mpsc channel buffer from 128 to 1024 messages to reduce backpressure blocking the process_stream receive loop. Replace sequential multi-subscriber fanout with try_join_all so that a slow outbound channel to one subscriber no longer delays delivery to the others. Add per-connection StreamMetrics with atomic counters reported once per second (msgs/sec, avg inbound wait, avg processing time) to make throughput and backpressure visible without per-message log overhead. Remove the tracing::instrument attribute from handle_message_from_slim to eliminate per-call span allocation overhead on the hot path. Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
Drop process_stream_message and its unused process_stream parameters (from_control_plane, require_header_mac) now that the loop calls handle_new_message directly. Move verify_remote_header_mac and verify_slim_header / TAG_LEN to Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
… decode prost decodes proto `string` fields with a UTF-8 validation scan and a heap allocation per field — six allocations + six UTF-8 scans per SLIMHeader just for the human-readable name components. Since routing uses EncodedName (XxHash64 integers) and only logging/display/HMAC paths need the string text, we can store the raw bytes and defer UTF-8 interpretation to the call sites that actually need &str. Change `StringName` fields from `string` → `bytes` in the proto, and configure prost via build.rs to generate `bytes::Bytes` (zero-copy) instead of the default `Vec<u8>`. The `bytes::Bytes` type holds a reference into the original network buffer — no heap allocation on the decode path. Wire format is unchanged: proto `string` and `bytes` share wire type 2 (length-delimited). Update call sites: - `from_strings()`: `s.into()` (String → Bytes, move with no copy) - `str_components()` + `Display`: use `from_utf8_unchecked` (safe: bytes always originate from valid UTF-8 via from_strings/parse_name) - `header_mac.rs`: deref to `[u8]` directly, no change needed - debug tamper path: rebuild via slice concat (Bytes is immutable) Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
… and update Cargo.lock Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
Replace nested EncodedName/NameId/StringName proto messages in Name with flat bytes fields (encoded_name: 40 bytes, str_name: packed strings). Prost decodes these as bytes::Bytes — zero-copy slices into the wire buffer — eliminating 6 nested-message decode calls per incoming message. Also convert ApplicationPayload.blob, SLIMHeader.identity, and SLIMHeader.version to bytes so prost generates bytes::Bytes instead of Vec<u8>/String, removing UTF-8 validation and heap allocation on decode. Zero-copy accessor fixes: - Add ProtoName::components_and_id() — decodes encoded_name once instead of twice when both components and id are needed (hot publish path was doing 10x from_le_bytes per message; now 5x) - Change SlimHeader::get_identity() to return &str borrowing from the Bytes field instead of allocating a new String on every call - Fix all components()+id() double-decode pairs in get_encoded_dst/source, subscription_table add/remove, forwarder, and message_processing Breaking wire-format change (perf branch only). Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
- Extract no_match_err() helper in subscription_table: 7 identical 4-line DataPathError::NoMatchEncoded constructions collapse to 1 call - Add try_str_components() private method; str_components() and Display for ProtoName share it, eliminating a duplicated unsafe block - from_strings: use AsRef<str> instead of Into<String> to avoid 3 intermediate String allocations per name construction - match_prefix: replace len() < 24 guard with is_empty() (encoded_name is always exactly 40 bytes or empty) - get_identity: remove unsafe from_utf8_unchecked; use checked from_utf8 since identity arrives as proto bytes and may not be valid UTF-8 - Remove get_version: dead code with zero callers Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
Store the `Content msg = 3` field in `Publish` as raw `bytes` instead of
a decoded `Content` message. Data-plane-only forwarding nodes that never
access the payload now avoid decoding the application/command payload on
every received message — the bytes are passed through unchanged.
Wire compatibility is preserved: proto3 `bytes` and `message` fields both
use wire type 2 (length-delimited), so the encoding on the wire is
identical.
Changes:
- proto: change `Content msg = 3` -> `bytes msg = 3` in Publish
- build.rs: add `.bytes(".dataplane.proto.v1.Publish")` for zero-copy Bytes
- utils.rs: ProtoPublish.get_payload() decodes on demand, returning
Option<Content>; set_payload() encodes to bytes; add consuming
into_application_payload / into_command_payload / into_*_payload
variants on Content and CommandPayload to avoid borrow-from-temporary
errors in callers
- update all callers across session, service, testing, and example layers
to use owned return types
Refs: #perf/e2e-improvements
Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
Replace Option<Name> source/destination fields in SLIMHeader with flat bytes fields, eliminating one level of indirection on every routing decision. The hot path `get_encoded_dst()` now calls `decode_name_bytes()` directly on the 40-byte Bytes field instead of unwrapping an Option<Name> and then reading its nested encoded_name field. The str_name components (used only for Display/Debug) move to new fields source_str/destination_str at tag numbers 12/13. Wire-breaking change: field 1/2 type changes from message to bytes. Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
… flatten Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
Remove methods that have no callers after the lazy-decode and flat-bytes proto changes: - ProtoPublish::is_command / get_application_payload / get_command_payload - SlimHeader::set_error_flag (duplicate of set_error) - ProtoMessage::set_error_flag (same) - ProtoName::name_id (redundant: id() + NULL_COMPONENT check) Also fix minor issues: - Redundant destructure in subscription_table remove_connection loop - Clippy: &str passed where str suffices in session_controller verify call Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
After moving api.rs to crates/proto/src/impls.rs and splitting the datapath crate, several fixups are needed: - crates/datapath/src/api.rs: remove EncodedName/NameId/StringName re-exports (removed from proto) and add re-exports of constants and helper functions (NULL_COMPONENT, id_to_string, encode_str_bytes, decode_str_bytes, etc.) from agntcy-slim-proto - crates/proto/src/impls.rs: promote encode_str_bytes/decode_str_bytes from pub(crate) to pub so they can be re-exported by the datapath crate; add bytes as a direct dependency - crates/testing: update receiver_app/sender_app to use get_payload() API instead of the removed msg.as_ref() pattern after lazy-decode change; add bytes dependency Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
Update integration_test.rs to use the new str_components() accessor instead of the removed str_name.as_ref() -> Option<StringName> pattern after the flat-bytes Name proto refactoring. Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
…tream Remove the per-connection StreamMetrics struct and companion reporter task that were added for profiling investigation. The profiling work is complete and these add overhead on the hot path. Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
e2e_header_sig was added at tag 12 on main. The source/dest string fields introduced in this branch are renumbered to tags 13 and 14 to avoid the collision, and e2e signing/verification is preserved. Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
Format all crates and sort testing/Cargo.toml to pass lint checks. Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
- decode_name_bytes: return None instead of panicking when encoded_name is non-empty but not exactly 40 bytes - decode_str_bytes: use checked indexing so a peer-supplied length prefix that overruns the buffer returns None rather than panicking - try_str_components: replace from_utf8_unchecked with from_utf8 since str_name can arrive over the wire and may not be valid UTF-8 Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
- Use id_from_str() instead of uuid::Uuid::parse_str() in reconciler, northbound, and southbound so that reserved string IDs like DATA_CHANNEL_ID are handled correctly instead of silently falling back to NULL_COMPONENT. - Replace .try_into().unwrap() with .try_into().ok()? in decode_str_bytes to make the function fully panic-free. - Add comment to mpsc::channel(1024) in message_processing.rs clarifying the buffer size is intentional for performance. Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
Signed-off-by: Sam Betts <1769706+Tehsmash@users.noreply.github.com>
4a43cf6 to
2d6c4cd
Compare
| // Raw-bytes encoding of a Content message. Stored as bytes so that | ||
| // data-plane-only forwarding nodes can pass it through without decoding. | ||
| // Wire-compatible with the original `Content msg = 3` field (same wire type 2). |
There was a problem hiding this comment.
I'd remove this line, as the comment refer to the previous Publish message definition.
| if slim_header.source.is_empty() { | ||
| return Err(MessageError::SourceEncodedNameNotFound); | ||
| } | ||
| match &slim_header.destination { | ||
| None => return Err(MessageError::DestinationNotFound), | ||
| Some(dst) if dst.name.is_none() => { | ||
| return Err(MessageError::DestinationEncodedNameNotFound); | ||
| } | ||
| _ => {} | ||
| if slim_header.destination.is_empty() { | ||
| return Err(MessageError::DestinationEncodedNameNotFound); | ||
| } |
There was a problem hiding this comment.
I think here we should also check the length of source/destination and we should also make sure that destination_str can be decoded as utf8.
If we don't do this, a peer sending a packet with a non-empty but invalid source or destination might trigger a panic in decode_name_bytes(&self.destination).expect("destination not set"), where we call expect() thinking the message was validated.
Otherwise we can remove the expect() and handle the error.
| ContentType::AppPayload(application_payload) => application_payload, | ||
| ContentType::CommandPayload(_) => panic!("the payload is not an application payload"), | ||
| /// Decodes the raw `msg` bytes into a [`Content`], returning `None` if absent. | ||
| pub fn get_payload(&self) -> Option<Content> { |
There was a problem hiding this comment.
It looks like this performs a new allocation on each call. It seems encrypt_message/decrypt_message decode the payload, then build_aad(msg) decodes it again just to read payload_type.
We could pass the already decoded payload_type into build_aad, as the caller already knows it.
There was a problem hiding this comment.
Here we do get_source()/get_dst(), and then we immediately take the components().
Do you think we can directly use get_encoded_source()/get_encoded_dst()?
There was a problem hiding this comment.
Here we do get_source()/get_dst(), and then we immediately take the components().
Do you think we can directly use get_encoded_source()/get_encoded_dst()?
| let comp_id = if route_name.id() == NULL_COMPONENT { | ||
| None | ||
| } else { |
There was a problem hiding this comment.
It looks like this pattern is repeated several times. Should we create a helper in ProtoName which does the same? Something like
impl ProtoName {
fn opt_string_id() {
if name.id() == NULL_COMPONENT { None } else { Some(name.string_id()) }
}
}| // encoded: presence byte + raw 40-byte flat encoding | ||
| if encoded.is_empty() { | ||
| buf.push(0); | ||
| } else { | ||
| buf.push(1); | ||
| buf.extend_from_slice(encoded); | ||
| } | ||
| // str_name: presence byte + u32 LE total length + raw packed bytes | ||
| if str_name.is_empty() { | ||
| buf.push(0); | ||
| } else { | ||
| buf.push(1); | ||
| buf.extend_from_slice(&(str_name.len() as u32).to_le_bytes()); | ||
| buf.extend_from_slice(str_name); |
There was a problem hiding this comment.
Shouldn't we reuse encode_str_bytes/decode_str_bytes here?
| if b.is_empty() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
The != 40 check already covers the is_ampty check, so I think we can remove if b.is_empty()...
Description
Performance improvements to the data-plane datapath focused on reducing allocation and decoding overhead on the hot forwarding path. Builds on top of the
feat/slimctl-bench-improvementsbranch.Changes
Data-plane throughput (profiling-driven)
OpenChannelbuffer from 1024 back to 128 (regression fix)Zero-copy proto field layout
Nameproto: replaced nestedEncodedName/NameId/StringNamemessage fields with flatbytes encoded_name(40 bytes: 3×u64 LE + u128 LE id) andbytes str_name(packed string components). Eliminates nested proto struct allocation on every name access.SLIMHeadersource/destination: replacedName source = 1/Name destination = 2message fields with flatbytes source = 1/bytes destination = 2(40-byte encoded name directly). Thestr_nameparts move to new fieldssource_str = 12/destination_str = 13. EliminatesOption<Name>unwrap and nested struct indirection on every routing decision.Publish.msg: changed fromContent msg = 3(message) tobytes msg = 3. Data-plane-only forwarding nodes now pass the application payload through without decoding it. Session-layer nodes decode on demand viaget_payload() -> Content.Dead code removal
ProtoPublish::is_command,get_application_payload,get_command_payload(zero callers post-refactor)set_error_flag(identical toset_error)ProtoName::name_id(no callers)Type of Change
Checklist