perf(node): cut ack re-encode, exact stanza sizing, warm session pre-filter - #1018
Merged
Conversation
…filter Three hot-path allocation cuts on the live message cycle: - Resolving a response waiter for an <ack> re-encoded the node (1 KiB marshal + Bytes copy + re-parse) even though every caller already has the node owned or shared. Split the entry points: the read-loop fast path hands its OwnedNodeRef straight into the waiter's Arc and shared callers clone the Arc. The borrow-only re-encode path had no remaining callers and is gone. - send_node serialized every stanza through the one-pass encoder with its 1 KiB default reserve; typical stanzas are a few hundred bytes. Use the exact two-pass sizing instead. - ensure_sessions built the buffer_unordered probe machinery even when every session was already cached. A synchronous try_has_session pre-filter short-circuits the warm case; contended or unknown entries still take the async probe.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR refactors ACK waiter delivery to reuse owned node representations, filters cached sessions before probing, and switches outgoing stanza serialization to exact-size marshaling. Tests cover ACK delivery, allocation behavior, server ACK events, and cached-session short-circuiting. ChangesACK delivery refactor
Session cache short circuit
Exact stanza marshaling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AckHandler
participant Client
participant ResponseWaiter
AckHandler->>Client: handle_ack_response_arc(Arc<OwnedNodeRef>)
Client->>Client: take_ack_waiter(ack_id)
Client->>ResponseWaiter: send existing Arc node
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
| Filename | Overview |
|---|---|
| src/client/messaging.rs | send_node now uses exact marshal sizing for outgoing stanzas. |
| src/client/node_io.rs | Ack handling now has owned and shared node entry points for waiter resolution. |
| src/client/sessions.rs | ensure_sessions_inner now skips async probing when the session cache already has every session. |
| src/handlers/basic.rs | The ack handler forwards the shared owned node to the ack waiter path. |
| src/client/tests.rs | Tests were updated for exact ack delivery behavior and warm-cache session short-circuiting. |
Reviews (2): Last reviewed commit: "refactor(ack): share the rejected-waiter..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🤖 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.
Inline comments:
In `@src/client/node_io.rs`:
- Around line 1143-1174: Deduplicate the identical rejected-waiter warning in
handle_ack_response_arc and handle_ack_response_owned by extracting it into a
small shared helper, then call that helper from both send error branches while
preserving the existing target, message, and rejected node ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 00f2cb11-6df4-4221-a971-a84be629d259
📒 Files selected for processing (5)
src/client/messaging.rssrc/client/node_io.rssrc/client/sessions.rssrc/client/tests.rssrc/handlers/basic.rs
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: Performance optimizations for ack handling, marshalling, and session caching, with added tests; low risk, well-contained changes.
Re-trigger cubic
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.
Summary
<ack>re-encoded the node (1 KiB marshal +Bytescopy + re-parse) even though every caller already has the node owned or shared. The read-loop fast path now hands itsOwnedNodeRefstraight into the waiter'sArcand shared callers clone theArc; the borrow-only re-encode entry point had no remaining callers and is removed.send_node: the one-pass encoder reserved its 1 KiB default for every outgoing stanza; typical live stanzas are a few hundred bytes. Switched to the existing exact two-pass path.ensure_sessions: thebuffer_unorderedprobe machinery was built even when every session was already cached. A synchronoustry_has_sessionpre-filter short-circuits the warm case; contended or unknown entries still take the async probe and the backend.Impact
DHAT ping-pong (2000 cycles, connect-subtracted, average of 2 runs, baseline = main @ d9141ac with AC power): allocation cost per cycle dropped from 36,248.3 to 35,169.6 bytes (-3.0%) and from 226.84 to 219.79 blocks (-3.1%). No extra retention at exit, 2000/2000 pongs with 0 lost in both runs. Timed CPU runs were inconclusive (hybrid-CPU run-to-run variance exceeds the effect size).
Cumulative since the start of this allocation series (pre-#1015 baseline of 46,088 B/cycle): -23.7% bytes per cycle.
Validation
cargo test -p whatsapp-rust --lib(976 passed; new tests: the Arc entry point delivers the original allocation viaArc::ptr_eq, the owned entry point resolves waiters, warm-cache ensure short-circuits while a cold probe still attempts the fetch)cargo test -p wacore --lib(1080 passed)cargo clippy -p whatsapp-rust -p wacore --lib --tests -- -D warningsRUSTFLAGS='--cfg getrandom_backend="wasm_js"' cargo check -p whatsapp-rust --lib --target wasm32-unknown-unknown --no-default-features