perf!: Arc<Event> event bus to eliminate deep clones on dispatch - #515
Conversation
CoreEventBus::dispatch now takes Event by value, wraps it in Arc once internally, and shares via Arc::clone to all handlers. Skips the Arc allocation entirely when no handlers are registered. EventHandler::handle_event receives Arc<Event> instead of &Event. BotEventHandler moves the Arc into the spawned task — no clone. ChannelEventHandler sends the Arc through the channel — no clone. on_event closure receives Arc<Event>; users match with &*event. MessageContext keeps owned fields so users can construct it from any source (events, DB, custom data). Users who need MessageContext clone explicitly in their handler; users who only read/filter events pay nothing. Lab results (experiments/heaptrack-impact-lab): - Core bus, 1 handler: -94.1% allocs, -98.8% bytes, -77.3% time - Core bus, 3 handlers: -94.1% allocs, -98.8% bytes, -87.5% time
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCore event ownership changed: CoreEventBus::dispatch now takes Event by value, wraps it once in Arc, and handlers receive Arc. Call sites, handler signatures, bot APIs, channels, and many tests were updated to use Arc and borrow matches (&*event). Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as Producer\n(Module / Client)
participant Core as CoreEventBus
participant HandlerA as Handler A
participant HandlerN as Handler N
Producer->>Core: dispatch(Event)
note right of Core: CoreEventBus::dispatch(event: Event)\nlet shared = Arc::new(event)
Core->>HandlerA: deliver(shared.clone())
Core->>HandlerN: deliver(shared.clone())
HandlerA->>HandlerA: handle_event(shared)
HandlerN->>HandlerN: handle_event(shared)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@claude review carefully, check how we can be dry, also more performatic and also improve the ergonomics of the API |
|
Claude finished @jlucaso1's task in 4m 4s —— View job Review: Arc<Event> event bus
The core design is solid — wrapping once in DRY1.
2. Both let ctx = MessageContext {
message: msg.clone(),
info: info.clone(),
client,
};This is the tax every impl MessageContext {
pub fn new(msg: &wa::Message, info: &MessageInfo, client: Arc<Client>) -> Self {
Self {
message: Box::new(msg.clone()),
info: info.clone(),
client,
}
}
}Or even better, one that works directly with the Arc'd event (see ergonomics section below). 3.
Performance1. The comment at
This is no longer true. With Also, line 51-52 says "Clones get their own 2. Everything else looks correct
API Ergonomics1. (Major) Add The match &*event {
Event::Message(msg, info) => {
let ctx = MessageContext {
message: msg.clone(),
info: info.clone(),
client,
};
// ...
}
}Add a constructor that does this in one call: impl MessageContext {
/// Extract from an Arc<Event> if it's a Message variant. Clones only the needed fields.
pub fn from_event(event: &Event, client: Arc<Client>) -> Option<Self> {
if let Event::Message(msg, info) = event {
Some(Self {
message: msg.clone(),
info: info.clone(),
client,
})
} else {
None
}
}
}Then the usage becomes: .on_event(|event, client| async move {
if let Some(ctx) = MessageContext::from_event(&event, client.clone()) {
handle_message(&ctx).await;
}
// handle other events with match &*event { ... }
})This keeps 2. (Major) E2E test double-match is painful — needs helper methods
Ok(ref event) if matches!(**event, Event::Message(_, _)) => {
if let Event::Message(msg, _) = &**event {
// finally can use msg
}
}This double-match (guard + inner if-let) exists because you can't destructure through Options to fix:
3. (Minor) The 4. (Minor) Consider Many e2e tests and the impl Event {
pub fn message_text(&self) -> Option<&str> {
if let Event::Message(msg, _) = self {
msg.conversation.as_deref()
} else {
None
}
}
}Summary
The PR is a solid perf improvement. The suggestions above are refinements, not blockers — the |
Benchmark Results59 unchanged benchmark(s)
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.rs (1)
106-113: 🧹 Nitpick | 🔵 TrivialConsider cloning
msg/infoonly when a response path is taken.
MessageContextconstruction currently deep-clones every incoming message. In this hot path, delaying clones until ping/media-ping branches helps retain the event-bus allocation win.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 106 - 113, Event::Message currently deep-clones msg and info up-front when constructing MessageContext; instead avoid the allocation by not cloning unless you need an owned response path: pass references (&msg, &info) into handle_message (or change MessageContext to hold references) and only clone into an owned MessageContext when entering the ping/media-ping response branches; adjust Event::Message handling and the MessageContext construction so clones happen inside the ping/media-ping branches (or inside handle_message when those branches are taken), keeping symbols Event::Message, MessageContext, handle_message, msg and info as the touchpoints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/benchmark.rs`:
- Around line 50-55: Event::Message currently clones msg and info
unconditionally when constructing MessageContext, which wastes work on the hot
path; change the code so msg.clone() and info.clone() are moved inside the
branch that actually needs owned data (e.g., the "ping" handling branch) and
only create MessageContext (and perform the clones) there. Locate the
Event::Message match arm and the MessageContext construction and defer cloning
of msg/info until you enter the branch that uses owned values (check the "ping"
branch), replacing the unconditional MessageContext creation with a branch-local
creation to preserve Arc/event-bus performance.
In `@src/pair.rs`:
- Around line 119-120: Remove the obsolete commented-out dispatch of Event::Qr
to reduce noise: delete the commented lines referencing
client.core.event_bus.dispatch(Event::Qr(Qr { codes })) and the accompanying
comment noting "We no longer dispatch the raw Event::Qr" so only the active,
current event handling remains; look for the Event::Qr and Qr symbols and the
client.core.event_bus.dispatch call to locate the exact commented block to
remove.
In `@wacore/src/types/events.rs`:
- Around line 229-237: The dispatch method is holding the RwLock read guard
while calling user handlers, which can block add_handler or deadlock if handlers
try to write to the bus; modify dispatch (the dispatch function that reads
self.handlers) to clone or collect the handler references (e.g., into a Vec or
Arc list) while the read lock is held, then drop the read guard before creating
the Arc<Event> and iterating to call handler.handle_event(Arc::clone(&event));
ensure you still share the same Arc<Event> instance with all handlers and only
call handle_event after the lock has been released.
---
Outside diff comments:
In `@src/main.rs`:
- Around line 106-113: Event::Message currently deep-clones msg and info
up-front when constructing MessageContext; instead avoid the allocation by not
cloning unless you need an owned response path: pass references (&msg, &info)
into handle_message (or change MessageContext to hold references) and only clone
into an owned MessageContext when entering the ping/media-ping response
branches; adjust Event::Message handling and the MessageContext construction so
clones happen inside the ping/media-ping branches (or inside handle_message when
those branches are taken), keeping symbols Event::Message, MessageContext,
handle_message, msg and info as the touchpoints.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ecc5f107-2bff-4661-aafb-43624d5fa57b
📒 Files selected for processing (28)
examples/benchmark.rssrc/bot.rssrc/client.rssrc/client/sessions.rssrc/features/chat_actions.rssrc/handlers/ib.rssrc/handlers/notification.rssrc/handlers/presence.rssrc/history_sync.rssrc/main.rssrc/message.rssrc/pair.rssrc/pair_code.rssrc/pdo.rssrc/receipt.rstests/e2e/src/lib.rstests/e2e/tests/app_state.rstests/e2e/tests/media.rstests/e2e/tests/messaging.rstests/e2e/tests/newsletter.rstests/e2e/tests/offline_groups.rstests/e2e/tests/offline_messages.rstests/e2e/tests/offline_receipts.rstests/e2e/tests/prekey_sessions.rstests/e2e/tests/profile.rstests/e2e/tests/profile_picture.rstests/e2e/tests/session_reuse.rswacore/src/types/events.rs
👮 Files not reviewed due to content moderation or server errors (3)
- src/handlers/notification.rs
- src/bot.rs
- tests/e2e/src/lib.rs
- Remove dead SharedData<T> (superseded by Arc<Event>) - Update LazyConversation comment (Arc dispatch gives parse-once for free) - Add Event::as_message() helper for clean destructuring - Add MessageContext::from_event() to eliminate construction boilerplate - Extract duplicated TestEventCollector to test_utils - Simplify double-match patterns in e2e tests with as_message()
- Defer MessageContext cloning until a handler branch actually needs it (benchmark.rs: only clone on "ping", main.rs: only clone on ping/media) - Release RwLock before calling handlers to prevent deadlock if a handler calls add_handler - Remove commented-out Event::Qr dispatch in pair.rs
|
@claude please review carefully again. I want your opinions about the changes, check the claims, and also suggest things that make sense. |
|
Claude finished @jlucaso1's task in 3m 18s —— View job Review: Arc<Event> event bus (round 2)
I read all 29 changed files and the full diff against Claim verification"Arc::new once, Arc::clone to all handlers" — Confirmed. "BotEventHandler moves the Arc into the spawned task — zero clone" — Confirmed. "ChannelEventHandler sends the Arc through the channel — zero clone" — Confirmed. "Skips Arc allocation entirely when no handlers are registered" — Confirmed. The "LazyConversation parse-once across handlers" — Confirmed. "Releases the handler RwLock before calling handlers" — Confirmed. Line 191-195 acquires read lock, Lab results (-94.1% allocations, -98.8% bytes) — Plausible. The old path cloned CorrectnessLock safety — Good. The
No soundness issues found. The DRY
Dead
API ergonomics
Suggestions1. (Minor) Consider Many e2e tests and both examples ultimately just need the message text. This is a 4-line helper that would clean up a lot of matching: impl Event {
pub fn message_text(&self) -> Option<&str> {
let (msg, _) = self.as_message()?;
msg.conversation.as_deref()
}
}This would simplify 2. (Minor) Current code: Ok(ref event) if event.as_message().is_some() => {
let (msg, _) = event.as_message().unwrap(); // can't fail, but reads awkwardlyThis could be: Ok(ref event) if let Some((msg, _)) = event.as_message() => {If 3. (Nit) The The doc example at What I would NOT change
SummaryThis is a clean, well-executed performance optimization. All claims check out. The previous review's concerns (lock safety, deferred cloning, dead code, DRY) have been addressed in follow-up commits. The |
- Add Event::message_text() for the common "get text content" pattern - Use if-let match guards to eliminate redundant as_message() + unwrap() - Simplify wait_for_text with message_text()
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
examples/benchmark.rs (1)
50-55:⚠️ Potential issue | 🟠 MajorDon't rebuild
MessageContextfor every message in the benchmark path.This eagerly clones
wa::Message/MessageInfobefore you know whether the message is"ping", so the benchmark starts measuring the deep-clone work thatArc<Event>is supposed to remove.♻️ Suggested hot-path fix
match &*event { Event::Message(_, _) => { - let ctx = MessageContext::from_event(&event, client).unwrap(); - - if let Some(text) = ctx.message.text_content() - && text == "ping" - { + if let Some((msg, info)) = event.as_message() + && msg.text_content() == Some("ping") + { + let ctx = MessageContext { + message: Box::new(msg.clone()), + info: info.clone(), + client, + }; info!("Received text ping, sending pong..."); let pong_text = format!("pong {}", ctx.info.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/benchmark.rs` around lines 50 - 55, The benchmark path is constructing MessageContext (via MessageContext::from_event) before verifying the message text, which forces deep clones of wa::Message/MessageInfo; change the hot path so you first inspect the Event::Message payload (borrow the message or use a non-cloning accessor) and check ctx.message.text_content() == "ping" (or equivalent) before calling MessageContext::from_event; only construct MessageContext after the text equals "ping" to avoid measuring the clone work—look for Event::Message, MessageContext::from_event, and ctx.message.text_content() to make this change.wacore/src/types/events.rs (1)
190-197:⚠️ Potential issue | 🔴 CriticalRelease the handler lock before invoking user code.
dispatch()still calls handlers while theRwLockread guard is alive. Any handler path that registers/removes handlers can stall or deadlock the bus; collect theArc<dyn EventHandler>values first, drop the guard, then iterate.🔧 Minimal fix
pub fn dispatch(&self, event: Event) { - let handlers = self.handlers.read().expect("RwLock should not be poisoned"); - if handlers.is_empty() { - return; - } + let handlers = { + let handlers = self.handlers.read().expect("RwLock should not be poisoned"); + if handlers.is_empty() { + return; + } + handlers.clone() + }; let event = Arc::new(event); - for handler in handlers.iter() { + for handler in handlers { handler.handle_event(Arc::clone(&event)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/types/events.rs` around lines 190 - 197, In dispatch, avoid holding the RwLock read guard while invoking user code: acquire the read lock on self.handlers, collect/cloned the Arc<dyn EventHandler> entries into a local Vec (e.g., Vec<Arc<dyn EventHandler>>), then drop the read guard before iterating; after the guard is released iterate over the collected handlers and call EventHandler::handle_event(Arc::clone(&event)) so registration/removal inside handlers cannot deadlock the bus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bot.rs`:
- Around line 39-46: Add a message-specific constructor to avoid the
downcast+Option from_event() forces callers to use when they already have an
Event::Message; implement a new pub fn from_message(message: &Message, info:
&MessageInfo, client: Arc<Client>) -> Self (or named from_parts) that constructs
and returns Self directly by boxing/cloning the provided message and cloning
info into the same fields used in from_event (mirrors message:
Box::new(msg.clone()), info: info.clone(), client), leaving from_event() as the
convenience wrapper that calls this new constructor after as_message().
In `@src/test_utils.rs`:
- Around line 127-147: The collector currently stores owned Event clones; change
TestEventCollector::events field to Mutex<Vec<Arc<Event>>> and update impl
EventHandler::handle_event to push a clone of the Arc<Event> (i.e.,
self.events.lock()...push(Arc::clone(&event))) instead of dereferencing and
cloning the Event; also change TestEventCollector::events() to return
Vec<Arc<Event>> (by cloning the Arc entries under the lock) so tests can observe
the shared Arc/LazyConversation without deep-cloning payloads.
---
Duplicate comments:
In `@examples/benchmark.rs`:
- Around line 50-55: The benchmark path is constructing MessageContext (via
MessageContext::from_event) before verifying the message text, which forces deep
clones of wa::Message/MessageInfo; change the hot path so you first inspect the
Event::Message payload (borrow the message or use a non-cloning accessor) and
check ctx.message.text_content() == "ping" (or equivalent) before calling
MessageContext::from_event; only construct MessageContext after the text equals
"ping" to avoid measuring the clone work—look for Event::Message,
MessageContext::from_event, and ctx.message.text_content() to make this change.
In `@wacore/src/types/events.rs`:
- Around line 190-197: In dispatch, avoid holding the RwLock read guard while
invoking user code: acquire the read lock on self.handlers, collect/cloned the
Arc<dyn EventHandler> entries into a local Vec (e.g., Vec<Arc<dyn
EventHandler>>), then drop the read guard before iterating; after the guard is
released iterate over the collected handlers and call
EventHandler::handle_event(Arc::clone(&event)) so registration/removal inside
handlers cannot deadlock the bus.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c003695-f6e6-414b-9916-ea792f1dd8f8
📒 Files selected for processing (8)
examples/benchmark.rssrc/bot.rssrc/handlers/notification.rssrc/main.rssrc/receipt.rssrc/test_utils.rstests/e2e/tests/offline_groups.rswacore/src/types/events.rs
…tructor - TestEventCollector stores Arc<Event> instead of deep-cloning events - Add MessageContext::from_parts() for direct construction from &Message + &Info - from_event() delegates to from_parts() - Update test patterns to deref through Arc
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/benchmark.rs`:
- Around line 50-56: The call to MessageContext::from_event(&event,
client).unwrap() in the Event::Message handling should avoid unwrap; replace it
with a safe check (e.g., use if let Some(ctx) =
MessageContext::from_event(&event, client) { ... } or match
MessageContext::from_event(...) { Some(ctx) => { /* existing handling */ }, None
=> { info!/warn!/error and return/continue } }) so the code handles the None
case gracefully and doesn't panic at runtime.
In `@src/main.rs`:
- Around line 106-111: The code calls MessageContext::from_event(&event,
client).unwrap() inside the Event::Message arm; replace the .unwrap() with an if
let to safely handle the Option: call MessageContext::from_event(&event, client)
and if it returns Some(ctx) await handle_message(&ctx), otherwise skip this
branch (e.g., continue the loop or log and return) so no panic can occur; keep
this change inside the same Event::Message arm that already checks
build_media_pong and PING_TRIGGER to preserve behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49b0ccd9-af58-4b2c-af3c-181e45922720
📒 Files selected for processing (6)
examples/benchmark.rssrc/main.rssrc/pair.rstests/e2e/src/lib.rstests/e2e/tests/offline_groups.rswacore/src/types/events.rs
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)
src/bot.rs (1)
428-435:⚠️ Potential issue | 🟡 MinorUpdate the
on_eventexample in rustdoc to dereferenceArc<Event>when pattern-matching.The
on_eventsignature now takesArc<Event>(line 430), but the example in thewith_pair_codedoc comment (lines 553–560) pattern-matcheseventby value. This will fail to compile because Rust does not auto-deref smart pointers likeArcin match patterns. The example must explicitly dereference:Fix
.on_event(|event, client| async move { - match event { + match &*event { Event::PairingCode { code, timeout } => { println!("Enter this code on your phone: {}", code); } _ => {} } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bot.rs` around lines 428 - 435, The rustdoc example for on_event uses pattern matching on `event` by value but the handler signature now receives `Arc<Event>`; update the example in the `with_pair_code` doc comment to dereference the Arc when matching (e.g., use `match &*event { ... }` or `match event.as_ref() { ... }` or `let Event::X = &*event => ...`) so the patterns match `Event` behind the `Arc<Event>`; edit the example in the doc comment near `with_pair_code` accordingly to reference `Arc<Event>` dereferencing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bot.rs`:
- Around line 428-435: The rustdoc example for on_event uses pattern matching on
`event` by value but the handler signature now receives `Arc<Event>`; update the
example in the `with_pair_code` doc comment to dereference the Arc when matching
(e.g., use `match &*event { ... }` or `match event.as_ref() { ... }` or `let
Event::X = &*event => ...`) so the patterns match `Event` behind the
`Arc<Event>`; edit the example in the doc comment near `with_pair_code`
accordingly to reference `Arc<Event>` dereferencing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 858245e7-66da-424b-863b-f05da44ed441
📒 Files selected for processing (4)
src/bot.rssrc/handlers/notification.rssrc/receipt.rssrc/test_utils.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 106-111: The guard is causing build_media_pong(msg) to run twice
on the media path; compute the media pong once and reuse it instead of
rebuilding inside handle_message. In the Event::Message arm, first call
build_media_pong(msg) and store the Option (e.g., media_pong), then use
media_pong.is_some() || msg.text_content() == Some(PING_TRIGGER) for the
conditional and pass the media_pong (or a flag) into MessageContext or
handle_message so handle_message doesn’t call build_media_pong again;
alternatively implement and use is_media_ping(&msg) (as suggested) in the guard
and only call build_media_pong once when creating the context or before invoking
handle_message. Ensure references to build_media_pong, handle_message,
MessageContext::from_parts and Event::Message are updated accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cdcc4c52-07c6-4006-aa32-785b356416ef
📒 Files selected for processing (2)
examples/benchmark.rssrc/main.rs
- Update with_pair_code doc example to use match &*event (Arc<Event>) - Inline handle_message into the event handler so build_media_pong runs once instead of twice (guard + handler)
Summary
CoreEventBus::dispatchtakesEventby value, wraps inArc::newonce internally, shares viaArc::cloneto all handlersRwLockbefore calling handlers (prevents deadlock if a handler callsadd_handler)EventHandler::handle_eventreceivesArc<Event>instead of&EventBotEventHandlermoves the Arc into the spawned task — zero cloneChannelEventHandlersends the Arc through the channel — zero cloneon_eventclosure receivesArc<Event>; users match withmatch &*event { ... }Event::as_message()andEvent::message_text()helpersMessageContext::from_parts()(direct construction) andfrom_event()(convenience wrapper)TestEventCollectorstoresArc<Event>instead of deep-cloning eventsSharedData<T>type, deadhandle_messagefunction, dead commented-outEvent::QrdispatchLazyConversationdocs —Arcdispatch gives parse-once across handlers for freeTestEventCollectorto sharedtest_utils&*eventmatchingWhy
The old bus dispatched
&Eventand every handler calledevent.clone()to own it for async tasks.Event::Message(Box<wa::Message>, MessageInfo)is expensive to deep-clone — the protobuf message has 40+ nested fields with Box allocations.With
Arc<Event>, the bus creates one shared allocation per dispatch. Each handler gets a cheapArc::clone(atomic increment, zero bytes allocated).Lab results (experiments/heaptrack-impact-lab)
Breaking changes
EventHandler::handle_eventtakesArc<Event>instead of&EventChannelEventHandler::new()returnsReceiver<Arc<Event>>instead ofReceiver<Event>on_eventclosure takesArc<Event>instead ofEvent— match with&*eventCoreEventBus::dispatchtakesEventby value instead of&EventMigration guide
Test plan
cargo fmt --allcargo clippy --all --testscargo test --all --exclude e2e-tests