perf(upload): slice ciphertext zero-copy instead of copying per attempt - #774
Merged
Conversation
The legacy in-memory upload closure did ciphertext[offset..].to_vec() on every send attempt — a full copy of the (potentially multi-MB) encrypted media payload on the common first attempt (offset 0), forced only because HttpRequest::with_body took Vec<u8>. Make HttpRequest.body a bytes::Bytes (with_body now takes impl Into<Bytes>, so existing Vec<u8> callers are unchanged) and hold the ciphertext as Bytes, so each attempt is ciphertext.slice(offset..) — a refcount bump, no copy. The ureq client reads the body via &body[..] (Bytes derefs to [u8]), so it needs no change. upload_stream already avoids this for large media; this fixes the buffered upload() path used for in-memory media.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR migrates HTTP request bodies from ChangesBytes-based HTTP request bodies
Sequence Diagram(s)sequenceDiagram
participant Upload as Client::upload
participant Ciphertext as Bytes~ciphertext~
participant Closure as send_body
participant Request as HttpRequest
Upload->>Ciphertext: wrap enc.data_to_upload as Bytes
Closure->>Ciphertext: slice(offset..) zero-copy
Closure->>Request: with_body(sliced_bytes)
Request->>Request: stores Option~Bytes~
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fb8d45bf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub method: String, // "GET" or "POST" | ||
| pub headers: HashMap<String, String>, | ||
| pub body: Option<Vec<u8>>, | ||
| pub body: Option<Bytes>, |
There was a problem hiding this comment.
When the full workspace is checked, tests/e2e/src/lib.rs still constructs HttpRequest { body: Some(code.as_bytes().to_vec()) }; after this public field changes to Option<Bytes>, that crate no longer type-checks. The patch updates the same direct construction in examples/benchmark.rs but misses the e2e helper, so cargo check --all/cargo test --all fail before any tests run.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/benchmark.rs (1)
117-122: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider using the builder pattern for consistency.
Look, the explicit
.into()works fine, but you're mixing direct struct construction with the builder pattern elsewhere in the codebase. The builder eliminates the need for that conversion call and keeps things cleaner:- let req = HttpRequest { - url: url.clone(), - method: "POST".into(), - headers: HashMap::new(), - body: Some(code.as_bytes().to_vec().into()), - }; + let req = HttpRequest::post(url.clone()) + .with_body(code.as_bytes().to_vec());The
with_bodymethod handles the conversion internally, which is the whole point of having it. Plus it's more maintainable when we make API changes like this Bytes migration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/benchmark.rs` around lines 117 - 122, The HttpRequest construction here uses direct field initialization with body: Some(code.as_bytes().to_vec().into()) which mixes styles with the builder used elsewhere; replace this with the HttpRequest builder (call new() or the existing builder for HttpRequest) and use the with_body(...) helper (which handles Bytes conversion) instead of manually converting the body, updating the code that creates req to use HttpRequest's builder and with_body to keep style and conversion centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@examples/benchmark.rs`:
- Around line 117-122: The HttpRequest construction here uses direct field
initialization with body: Some(code.as_bytes().to_vec().into()) which mixes
styles with the builder used elsewhere; replace this with the HttpRequest
builder (call new() or the existing builder for HttpRequest) and use the
with_body(...) helper (which handles Bytes conversion) instead of manually
converting the body, updating the code that creates req to use HttpRequest's
builder and with_body to keep style and conversion centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 206f2714-2b1b-4d95-ab89-8d7e923e3795
📒 Files selected for processing (3)
examples/benchmark.rssrc/upload.rswacore/src/net.rs
Benchmark Results67 unchanged benchmark(s)
|
The e2e helper builds an HttpRequest struct literal directly; its body field is now Option<Bytes>, so the Vec<u8> needs .into(). Missed in the first pass (the benchmark example had the same literal and was fixed). This is what failed Build & Test / E2E / Integration Benchmark on the PR.
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.
The legacy in-memory upload closure ran
ciphertext[offset..].to_vec()on every send attempt — a full copy of the (potentially multi-MB) encrypted media payload on the common first attempt (offset 0). It was forced only becauseHttpRequest::with_bodytookVec<u8>; the closure is re-callable for retry/resume so the ciphertext can't be moved out, and theVec<u8>body type required an owned copy.This makes
HttpRequest.bodyabytes::Bytes(with_bodynow takesimpl Into<Bytes>, so existingVec<u8>callers are unchanged) and holds the ciphertext asBytes, so each attempt becomesciphertext.slice(offset..)— a refcount bump, no copy. The ureq client reads the body via&body[..](Bytesderefs to[u8]), so it needs no change.upload_streamalready avoids the copy for large media; this fixes the bufferedupload()path that's still the public API for in-memory media. Pure perf/type change; the existing upload retry/failover tests exercise the multi-attempt body path.