perf(hooks): add subscription-based event filtering to reduce hot-path dispatch overhead#2162
perf(hooks): add subscription-based event filtering to reduce hot-path dispatch overhead#2162coleleavitt wants to merge 2 commits into
Conversation
…h dispatch overhead Replace sequential all-hooks dispatch with subscription-based filtering. Each hook declares which event types it cares about via HOOK_SUBSCRIPTIONS. Critical hooks (claudeCodeHooks, stopContinuationGuard, writeExistingFileGuard) remain awaited; 18 other hooks fire-and-forget with error logging. During LLM streaming (~100 message.part.delta events/sec), 19 of 21 hooks are now skipped entirely, reducing async dispatch from ~2100/sec to ~200/sec.
There was a problem hiding this comment.
2 issues found across 1 file
Confidence score: 2/5
- Stateful hooks no longer receive
session.deletedevents insrc/plugin/event.ts, which is a concrete regression that can leak resources during cleanup - Opencode compatibility is broken because
session.compactedis filtered out by the new subscription map, so compaction handlers stop running - Given both issues are high severity and user-facing, the merge risk is high despite limited file scope
- Pay close attention to
src/plugin/event.ts- event filtering removessession.deletedandsession.compacteddelivery to hooks.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/plugin/event.ts">
<violation number="1" location="src/plugin/event.ts:140">
P1: Custom agent: **Opencode Compatibility**
Opencode compatibility break: `session.compacted` is filtered out by the new subscription map, so hooks that implement compaction handling no longer receive that OpenCode event.</violation>
<violation number="2" location="src/plugin/event.ts:159">
P1: Stateful hooks are filtered from receiving session.deleted events they need for cleanup, causing resource leaks</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…cription map - Add session.compacted to SESSION_LIFECYCLE (it's a lifecycle event) - Add session.deleted to hooks that do cleanup: contextWindowMonitor, compactionTodoPreserver, writeExistingFileGuard, todoContinuationEnforcer, directoryReadmeInjector, thinkMode - Add session.compacted to hooks that reset state on compaction: compactionTodoPreserver, atlasHook, directoryAgentsInjector, directoryReadmeInjector, rulesInjector - Fix sync throw escaping fire-and-forget catch by wrapping invoke in Promise.resolve().then() instead of Promise.resolve(invoke())
|
[sisyphus-bot] PR Assessment: Performance Optimization with Event FilteringType: Performance enhancement (not a bugfix) SummaryThis PR introduces subscription-based event filtering to reduce hot-path dispatch overhead during LLM streaming. Instead of dispatching all events to all 21 hooks sequentially, hooks now only receive events they've subscribed to. Key Changes
Cubic AI Review FindingsCubic raised 2 P1 concerns:
Analysis of findings:
The Cubic findings may refer to an earlier commit state. The PR description mentions "fixes lifecycle subscriptions" suggesting these were addressed. Merge Readiness Check
Blockers to Auto-Merge
Suggested ActionNeeds human maintainer review. This is a core architectural change to the event dispatch system. While CI passes and the code looks correct, the performance optimization touches critical hot-path code that affects all hook execution. Please have a maintainer:
Assessment by sisyphus-bot |
|
[sisyphus-bot] PR sweep first-pass triage on
Needs rebase + review. Please rebase onto current Assigning |
|
[sisyphus-bot] Hi coleleavitt. 🙏 Thanks for the subscription-based event filtering perf work. Picking this back up from the 5/16 triage. The PR is small (71/21 in 1 file) and shows CONFLICTING against current Could you rebase against current |
|
no longer contributing atm; stale PR and can be rebased if desired |
eb25d29 to
2bfad49
Compare
Summary
subscriptionsreceive all events as beforeProblem
During LLM streaming, the processor emits ~10 PartDelta events/second. Each event dispatches to ALL 21 hooks sequentially, even though most hooks don't handle delta events. This creates ~2,100 unnecessary async dispatches/second, generating excessive garbage that triggers GC pressure and contributes to TUI freeze.
Solution
Hooks can now declare which event types they care about via a
subscriptionsarray:The dispatcher builds a subscription map at initialization and routes events only to subscribed hooks. Hooks without
subscriptionsare treated as "subscribe to all" for backward compatibility.Changes
src/plugin/event.ts: AddedSubscriptiontype,subscriptionsfield toHook, subscription map building increate(), filtered dispatch indispatch()Verification
bun x tsc --noEmitpasses cleanSummary by cubic
Adds subscription-based event filtering so only subscribed hooks run. This reduces hot-path dispatch during LLM streaming and lowers GC pressure, and fixes lifecycle subscriptions and fire-and-forget error handling.
Refactors
Bug Fixes
Written for commit cae1645. Summary will update on new commits.