perf(events): snapshot handlers behind an Arc to drop the per-event Vec clone - #719
Merged
Conversation
…ec clone CoreEventBus::dispatch cloned the whole Vec<Arc<dyn EventHandler>> out of the RwLock on every event (a heap allocation plus one atomic bump per handler), and the interest precheck only ran after that clone, so an event no handler wanted still paid for it. The handler list is append-only and only mutated at startup. Store the handlers in an immutable snapshot behind an Arc so dispatch clones just the outer Arc (one refcount bump, zero Vec allocation), and cache the OR of every handler's interest in that snapshot so an ignored kind short-circuits on a single bitmask test before touching anything else. add_handler rebuilds and swaps the snapshot copy-on-write, keeping registration order. has_handlers and has_handler_for now read the snapshot instead of locking and scanning.
|
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
WalkthroughCoreEventBus now uses immutable copy-on-write HandlerSnapshot objects stored as Arc<RwLock<Arc>>. add_handler rebuilds and swaps snapshots under a write lock; dispatch clones the outer snapshot Arc, drops locks, checks a cached aggregate interest, and iterates a stable ordered handler list. Tests cover interest widening, OR semantics, ordering, and reentrancy-safe registration. ChangesHandler Snapshot Mechanism
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: 0c4999cd87
ℹ️ 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".
Benchmark Results67 unchanged benchmark(s)
|
Drop the cached interest aggregate computed at add_handler: it short-circuited dispatch using a stale snapshot, so a handler whose interest() widens at runtime stopped receiving the newly-wanted kinds (the pre-change code re-evaluated interest every dispatch). Keep the Arc handler snapshot (the actual win: no per-event Vec clone) and re-evaluate interest over it in dispatch and has_handler_for. Adds a regression test for a handler that widens its interest after registration.
36 tasks
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.
Problem
CoreEventBus::dispatchis on the hottest paths in the client (one call perinbound message in
message.rs, one per receipt inreceipt.rs, plus everynotification, presence, group and app-state update). On each event it cloned the
entire
Vec<Arc<dyn EventHandler>>out of theRwLock: that is a heapallocation for the backing buffer plus one atomic refcount bump per registered
handler, every single time. Worse, the interest precheck ran after the clone,
so an event that no handler was interested in still paid the full clone before
being dropped.
The handler list is append-only and in practice only mutated at startup (every
register_handler/add_handlercall), so paying a per-event allocation for alist that never changes during steady state is pure overhead.
Change
Store the handlers in an immutable
HandlerSnapshotheld behind anArc(
RwLock<Arc<HandlerSnapshot>>). Dispatch now clones only the outerArc(onerefcount bump, zero
Vecallocation), then drops the lock before iterating.The snapshot also caches the OR of every handler's
EventInterestas a singleaggregate bitmask.
dispatchtestsaggregate.wants(kind)first, so an eventno handler wants short-circuits on one bitmask check and materializes nothing.
has_handler_forandhas_handlersread this cached snapshot too, droppingtheir previous lock-and-scan over the handler list.
add_handlerrebuilds the snapshot copy-on-write under the write lock and swapsit in, OR-ing the new handler's interest into the aggregate. Registration order
is preserved, so observable handler ordering is unchanged.
Re-entrancy and concurrency are safe by construction:
dispatchonly holds theread lock long enough to clone the outer
Arc, then releases it. A concurrentadd_handlerswaps in a freshArc, but the snapshot a dispatch already clonedkeeps it alive and unchanged for the duration of that dispatch, so an iteration
never observes a mutated list and a handler added mid-dispatch is not invoked
for the in-flight event.
Tests
All existing event tests stay green. Added:
aggregate_interest_and_has_handler_for: the cached aggregate is the OR ofthe registered handlers' interests and answers
has_handler_for/has_handlerscorrectly, including the empty-bus case.dispatch_preserves_handler_ordering: copy-on-write rebuilds keep handlers inregistration order.
dispatch_is_reentrancy_safe_against_concurrent_add: a handler that registersanother handler while it is being dispatched does not deadlock, the new
handler is not invoked for the in-flight event, and the next dispatch sees
both. This locks in the snapshot-outlives-swap guarantee.
interest_filters_dispatchalready proves an event no handlerwants invokes nothing; that path now exercises the cached aggregate
short-circuit.
Performance
Per-event cost on the hot dispatch path drops from "allocate a
Vec+ N atomicbumps + post-clone interest scan" to "one atomic bump + one bitmask test", and
an ignored-kind event now costs a single bitmask test with no allocation at all.
The added cost moves to
add_handler, which rebuilds the snapshot, but that runsonly at startup.