perf(handlers): shrink the inbound non-message stanza path; inbound_stanza bench - #1399
Merged
Conversation
Three independent changes on the read loop's non-message path, plus the benchmark that keeps them from silently reverting. All numbers below come from the new `benches/inbound_stanza`, release, on one box; the honest metric is bytes per stanza, not wall time. `NotificationHandler`'s future is boxed once per inbound `<notification>` by `async_trait`, and every arm of `handle_notification_impl` was awaited unboxed, so that one allocation was sized for the union of all of them: a `<notification type="picture">` paid for `handle_devices_notification`'s locals. The file's own comment claimed separate async fns already avoided this. They do not — awaiting a plain async fn inlines its state machine into the caller's. Boxing the asynchronous arms takes a notification from 2306 B to 146 B (with a subscriber, 2850 B to 691 B), and a 500-notification drain from 2850 B to 690 B per item, so a reconnect draining 500 of them sheds ~1.1 MB of transient allocation. The allocation *count* is unchanged and wall time moved 540 -> 471 ns median, inside this fixture's run-to-run spread: this is allocation volume, not latency. `handle_receipt_inline` ran its `has_handler_for(EventKind::Receipt)` bail only after parsing the whole stanza — `from`, `id`, `participant`, `recipient`, `participant_pn`, `offline`, `t`, and the feature-incapable child scan. It now reads `type` first and bails on that. Gating on the raw type is behaviour-preserving because `downgrade_for_feature_incapable` only ever rewrites Delivered to Sent and so can never produce the Retry the gate lets through; a new test in `wacore/src/stanza/receipt.rs` pins that over every variant. This path runs inline on the read loop exactly when nothing is subscribed, so it is read-loop time: 383 -> 345 ns median per ignored receipt, small and at the edge of the noise. `CoreEventBus::dispatch_with(kind, || event)` (purely additive) builds the payload only when a handler is registered. Applied where the payload is more than a couple of `Jid` clones: `GroupUpdate` (a whole `GroupNotificationAction` with its participant `Vec`, once per action per notification), `DeviceListUpdate`, `IdentityChange`, `BusinessStatusUpdate`, `MexNotification` and `ContactNumberChanged`. Cheap producers keep plain `dispatch`. It does not show up in the benchmark rows above — none of their stanzas reaches those sites — because the payoff is proportional to the payload and lands on a consumer that does not subscribe to that kind. The guard: `benches/inbound_stanza` (feature `bench-harness`, so CodSpeed's client shard picks it up) covers `<receipt>`, `<presence>`, `<notification>` and `<ack>` with and without a subscriber, plus a 500-stanza burst per kind, reporting bytes and allocations per stanza through divan's `AllocProfiler`. What that cannot gate is the size of a single block, which is the whole regression here, so the ceiling lives in a unit test instead: `notification_future_is_not_sized_for_every_arm` asserts the largest single block a notification allocates stays under 1 KiB. It was verified to fail, reporting 2272 bytes, with the arms un-boxed. The crate's counting test allocator gained a max-block tracker to support it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172fpxasGTrouFyYH5UGmjN
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe PR adds lazy event construction, boxes asynchronous notification handlers, moves receipt filtering earlier, and adds allocation checks. It also adds a receive harness and Divan benchmarks for receipt, presence, notification, and ack stanzas. ChangesInbound stanza flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change moves receipt filtering earlier while preserving retry handling and missing-ID reporting, reducing unnecessary inbound processing without an identified merge-readiness risk. Sequence Diagram(s)sequenceDiagram
participant NotificationHandler
participant CoreEventBus
participant EventHandler
NotificationHandler->>CoreEventBus: dispatch_with(EventKind, builder)
CoreEventBus->>CoreEventBus: Check handler interest
CoreEventBus->>EventHandler: Dispatch built Event
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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/receipt.rs | Moves receipt type and missing-ID validation before the no-subscriber gate, completing the previously requested diagnostic fix. |
| src/handlers/notification/mod.rs | Boxes individual asynchronous notification arms to prevent their state machines from inflating the outer async-trait future. |
| wacore/src/types/events.rs | Adds subscriber-gated event construction through dispatch_with. |
| src/handlers/notification/groups.rs | Uses lazy event construction for group and MEX notification payloads. |
| src/handlers/notification/device.rs | Uses lazy event construction for identity and device-list updates. |
| benches/inbound_stanza.rs | Adds allocation-focused benchmarks for receipts, presence, notifications, acknowledgements, and burst processing. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Inbound stanza] --> B{Stanza type}
B -->|Receipt| C[Parse type and required ID]
C --> D{Retry or Receipt subscriber?}
D -->|No| E[Return without full payload parse]
D -->|Yes| F[Parse and process receipt]
B -->|Notification| G[Select notification type]
G --> H[Box only the selected async arm]
H --> I{Matching event subscriber?}
I -->|No| J[Skip event payload construction]
I -->|Yes| K[Build and dispatch event]
Reviews (2): Last reviewed commit: "fix(receipt): keep the missing-id warnin..." | 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}
A receipt without an `id` is a protocol error worth its warning whether or not anything is subscribed, and the early gate had moved that check behind itself. `id` is now the one other attribute read before the gate: a single inline copy, so the ignored-receipt path still parses two attributes and not the stanza. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172fpxasGTrouFyYH5UGmjN
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
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
2 participants
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 non-message half of the second parallel exploration (receipts, presence, notifications, acks and the event bus), after #1388–#1396. The event bus itself measured as already optimal (0 allocations with no subscriber, exactly 1
Arc<Event>with any number); what was left is on the producers. All numbers below are from the newbenches/inbound_stanza, release, one box; the honest metric is bytes per stanza, not wall time.The notification handler's future was sized for every arm
async_traitboxesNotificationHandler's future once per inbound<notification>, and every arm ofhandle_notification_implwas awaited unboxed. Awaiting a plain async fn inlines its state machine into the caller's — the file's own comment claimed separate async fns already avoided this, and they do not — so that one allocation was the union of all seventeen arms: a<notification type="picture">paid forhandle_devices_notification's locals. The asynchronous arms are nowBox::pinned; only the arm that runs pays its own small block.<notification type="picture">A reconnect draining 500 notifications sheds ~1.1 MB of transient allocation. Allocation count is unchanged and wall time (540 → 471 ns median) is inside the run-to-run spread. A unit test (
notification_future_is_not_sized_for_every_arm) pins the largest single block under 1 KiB through the realStanzaHandler::handle; it was verified to fail at 2272 bytes with the arms un-boxed, so a future.awaiton an unboxed arm is caught in nextest.The receipt subscriber gate runs before the parse
handle_receipt_inlineran itshas_handler_for(EventKind::Receipt)bail only after parsingfrom,id,participant,recipient,participant_pn,offline,tand the feature-incapable child scan. It now readstypeandidfirst and bails on the type (the missing-idwarning stays ahead of the gate, so a malformed receipt is still reported whether or not anything is subscribed). Gating on the raw type is behaviour-preserving becausedowngrade_for_feature_incapableonly ever rewrites Delivered to Sent and can never produce the Retry the gate lets through; a new test inwacore/src/stanza/receipt.rspins that over every variant. This path runs inline on the read loop exactly when nothing is subscribed: 383 → 345 ns per ignored receipt, small and at the edge of noise.Payloads built only for a listener
CoreEventBus::dispatch_with(kind, || event)(additive) builds the payload only when a handler forkindis registered.dispatchalready costs nothing without one, but the payload has been built by then. Applied where it is more than a couple ofJidclones:GroupUpdate(a wholeGroupNotificationActionwith its participantVec, once per action per notification),DeviceListUpdate,IdentityChange,BusinessStatusUpdate,MexNotification,ContactNumberChanged. Cheap producers keep plaindispatch. Not in the table above (none of its stanzas reaches those sites); the payoff is proportional to the payload, on a consumer that does not subscribe to that kind.The guard
benches/inbound_stanzaonReceiveHarness(featurebench-harness, so the CodSpeed client shard runs it):<receipt>,<presence>,<notification>and<ack>with and without a subscriber, plus a 500-stanza burst per kind, reporting bytes and allocations per stanza through divan'sAllocProfiler.Not in this PR
size_of::<Event>()is 528 B because ofGroupUpdate(528) andIncomingCall(432), so every dispatched event is a 544 BArcregardless of payload; boxing those two takes it to ~280 B but changes the frozenEventsurface, so it comes as its ownperf(events)!PR.Verification
cargo clippy -p wacore -p whatsapp-rust --all-targets --features bench-harness -- -D warningscleancargo test -p whatsapp-rust --lib,cargo test -p wacore --libpass, including the two new testsCI note:
Semver Checks (informational)is red onmainas well and is not this PR's; no fix exists for it in this branch.🤖 Generated with Claude Code
https://claude.ai/code/session_0172fpxasGTrouFyYH5UGmjN