perf: Only compute checksums on debug builds #2446
Conversation
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
WalkthroughAdds cfg(debug_assertions)-gated checksum behavior in two_part codec: debug builds compute/verify xxh3_64 over header+payload (only if provided checksum != 0); release builds write 0 and skip hashing/verification, preserving layout and sizes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Encoder
participant Stream
Caller->>Encoder: encode(header, data)
alt debug_assertions
Encoder->>Encoder: compute xxh3_64(header+data)
Encoder->>Stream: write header, data, checksum
else release
Encoder->>Stream: write header, data, checksum=0
end
sequenceDiagram
participant Stream
participant Decoder
participant Caller
Stream->>Decoder: read header, data, checksum
alt debug_assertions and checksum != 0
Decoder->>Decoder: compute xxh3_64(header+data)
alt match
Decoder-->>Caller: deliver message
else mismatch
Decoder-->>Caller: error(ChecksumMismatch)
end
else (release or checksum==0)
Decoder-->>Caller: deliver message (no verification)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
lib/runtime/src/pipeline/network/codec/two_part.rs (3)
70-76: Consider guarding total_len computation against overflowWhile pre-existing, total_len = 24 + header_len + body_len can overflow on malformed input. Using checked_add here allows returning a clean error rather than risking panics later.
Rust snippet (applies outside the changed block):
let total_len = 24usize .checked_add(header_len) .and_then(|v| v.checked_add(body_len)) .ok_or_else(|| TwoPartCodecError::InvalidMessage("total_len overflow".to_string()))?;
18-18: Gate xxhash import to avoid unused_imports in release builds — fix requiredxxh3_64 is only used inside #[cfg(debug_assertions)] blocks but the import is unconditional; in release builds the import will be unused and can trigger warnings (CI runs cargo clippy with -D warnings).
Files to update:
- lib/runtime/src/pipeline/network/codec/two_part.rs
- line 18: unconditional import
use xxhash_rust::xxh3::xxh3_64;- lines ~94 and ~138: uses of
xxh3_64are inside#[cfg(debug_assertions)]- CI: .github/workflows/build-and-test.yml and .github/workflows/pre-merge-rust.yml run
cargo clippy ... -D warningsSuggested change:
-use xxhash_rust::xxh3::xxh3_64; +#[cfg(debug_assertions)] +use xxhash_rust::xxh3::xxh3_64;
452-480: Gate checksum-mismatch tests to debug mode to avoid failures undercargo test --releaseShort: encode/decode compute/verify checksum only under #[cfg(debug_assertions)], so tests that assert ChecksumMismatch will fail in release builds. I inspected lib/runtime/src/pipeline/network/codec/two_part.rs and verified the guards; CI workflows (.github/workflows/build-and-test.yml, .github/workflows/pre-merge-rust.yml) run
cargo testwithout--release, but this change is recommended to avoid local/other CI--releaseruns.Files needing attention:
- lib/runtime/src/pipeline/network/codec/two_part.rs
- fn test_checksum_mismatch() (around lines 452-480)
- async fn test_streaming_corrupted_data() (around lines ~643-672)
Apply these corrections (note: keep the test attribute; add the cfg above it):
-#[test] -fn test_checksum_mismatch() { +#[cfg(debug_assertions)] +#[test] +fn test_checksum_mismatch() { ... }-#[tokio::test] -async fn test_streaming_corrupted_data() { +#[cfg(debug_assertions)] +#[tokio::test] +async fn test_streaming_corrupted_data() { ... }Please apply the change (or confirm if you intentionally want these tests to run under release builds).
🧹 Nitpick comments (2)
lib/runtime/src/pipeline/network/codec/two_part.rs (2)
129-145: Avoid extra copy during checksum computation in debug modeThe current approach allocates a new buffer and copies header+data just to hash. Compute the checksum incrementally to eliminate the copy.
// Only compute the checksum in debug mode. // If we're in release mode, put a dummy value. #[cfg(debug_assertions)] { - // Compute checksum of the data - let mut data_to_hash = BytesMut::with_capacity(header_len + body_len); - data_to_hash.extend_from_slice(&item.header); - data_to_hash.extend_from_slice(&item.data); - let checksum = xxh3_64(&data_to_hash); - - dst.put_u64(checksum); + // Compute checksum over header and data without extra allocation + let mut hasher = xxhash_rust::xxh3::Xxh3::new(); + hasher.update(&item.header); + hasher.update(&item.data); + dst.put_u64(hasher.digest()); } #[cfg(not(debug_assertions))] { dst.put_u64(0); }
66-69: Prevent unused variable warning in release by conditionally binding checksumIn release builds, checksum is currently read but unused. Bind it conditionally to avoid an unused variable warning.
Rust snippet (applies outside the changed block):
// Replace: let checksum = cursor.get_u64(); // With: #[cfg(debug_assertions)] let checksum = cursor.get_u64(); #[cfg(not(debug_assertions))] let _ = cursor.get_u64(); // still advance the cursor by 8 bytes
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/runtime/src/pipeline/network/codec/two_part.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test - dynamo
- GitHub Check: pre-merge-rust (.)
- GitHub Check: pre-merge-rust (lib/bindings/python)
rmccorm4
left a comment
rmccorm4
left a comment
There was a problem hiding this comment.
LGTM - but looks like a good suggestion from coderabbit: https://github.com/ai-dynamo/dynamo/pull/2446/files#r2277430507
Extent of my benchmarking so far has been with nvtx. Added a couple nvtx regions, ran |
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com> Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
As pointed out by @ryanolson, we're computing checksums for all our messages. We should only be doing this on debug builds. From some preliminary benchmarking, seems like this could reduce our message send/recv overhead by ~25%.
Summary by CodeRabbit