From c0b6c2a1e51ad3bfebf561cb76e6d151cde49edb Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 3 May 2026 07:07:11 -0500 Subject: [PATCH] perf(hooks): fast-path skip when no hooks configured (#455 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that `tool_call_before` / `tool_call_after` fire on every tool dispatch, the cost of constructing a `HookContext` (which allocates for `workspace`, `model`, `session_id`, …) shows up on the hot path even when the user has zero hooks configured — the common case. Adds `HookExecutor::has_hooks_for_event(event)` as a cheap boolean gate that callers consult before building the context. The pre-check returns false when: * `config.enabled == false` (globally disabled). * No hook in the config has the given `event`. Wired through every fire site: * `tool_routing.rs::handle_tool_call_started` — `ToolCallBefore`. * `tool_routing.rs::handle_tool_call_complete` — `ToolCallAfter`. Also skips the `result.content.clone()` that the `with_tool_result` builder demands. * `ui.rs::dispatch_user_message` — `MessageSubmit`. * `ui.rs::apply_engine_error_to_app` — `OnError`. Inside `HookExecutor::execute` itself, also short-circuit before calling `context.to_env_vars()` when no hooks match the event — defends against a caller that builds the context but forgets to gate. Tests: 3 new tests cover empty-config / globally-disabled / per-event filtering. The existing 18 hook tests pass unchanged. No behavioral change for users with hooks configured; pure allocation-free fast path otherwise. --- CHANGELOG.md | 6 ++- crates/tui/src/hooks.rs | 73 ++++++++++++++++++++++++++++++ crates/tui/src/tui/tool_routing.rs | 32 +++++++------ crates/tui/src/tui/ui.rs | 10 +++- 4 files changed, 105 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24e8ec9b..624c8724 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -266,7 +266,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 because it needs a synchronous-gate contract that doesn't exist today. Combined with the existing `session_start` / `session_end` / `mode_change` events, every variant in the - `HookEvent` enum now has a live producer. + `HookEvent` enum now has a live producer. Each fire is + fast-path-gated by + `HookExecutor::has_hooks_for_event(event)` so per-tool + dispatch never pays for `HookContext` allocation when the + user has no hooks configured (the common case). - **RLM tool family** (#512) — `rlm` tool cards map to `ToolFamily::Rlm` and render `rlm`, not `swarm`. Stale "swarm" wording cleaned out of docs / comments / tests. diff --git a/crates/tui/src/hooks.rs b/crates/tui/src/hooks.rs index 3c4ce48f..76234aab 100644 --- a/crates/tui/src/hooks.rs +++ b/crates/tui/src/hooks.rs @@ -481,6 +481,17 @@ impl HookExecutor { &self.session_id } + /// Cheap pre-check: are there any enabled hooks for this event? + /// Lets call sites avoid building a [`HookContext`] (which allocates + /// for `workspace`, `model`, `session_id`, …) on every tool call + /// when the user hasn't configured any hooks. The cost matters + /// because `ToolCallBefore` / `ToolCallAfter` fire from + /// `tool_routing.rs` on every tool dispatch (#455). + #[must_use] + pub fn has_hooks_for_event(&self, event: HookEvent) -> bool { + self.config.enabled && self.config.hooks.iter().any(|h| h.event == event) + } + /// Execute all hooks for an event pub fn execute(&self, event: HookEvent, context: &HookContext) -> Vec { if !self.config.enabled { @@ -488,6 +499,14 @@ impl HookExecutor { } let hooks = self.config.hooks_for_event(event); + if hooks.is_empty() { + // Fast path: no hooks for this event → skip the + // `context.to_env_vars()` HashMap allocation. With + // `tool_call_before` / `tool_call_after` firing per-tool + // (#455) this allocation would otherwise happen on every + // tool dispatch even for users with zero hooks configured. + return Vec::new(); + } let env_vars = context.to_env_vars(); let mut results = Vec::new(); @@ -820,4 +839,58 @@ mod tests { assert!(executor.session_id().starts_with("sess_")); assert_eq!(executor.session_id().len(), 13); // "sess_" + 8 chars } + + #[test] + fn has_hooks_for_event_fast_path_returns_false_for_empty_config() { + let executor = HookExecutor::disabled(); + // No hooks configured AT ALL — every event is a fast skip. + for event in [ + HookEvent::SessionStart, + HookEvent::SessionEnd, + HookEvent::MessageSubmit, + HookEvent::ToolCallBefore, + HookEvent::ToolCallAfter, + HookEvent::ModeChange, + HookEvent::OnError, + ] { + assert!( + !executor.has_hooks_for_event(event), + "empty config must short-circuit for {event:?}" + ); + } + } + + #[test] + fn has_hooks_for_event_returns_false_when_globally_disabled() { + let config = HooksConfig { + enabled: false, + hooks: vec![Hook::new(HookEvent::ToolCallBefore, "echo blocked")], + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, PathBuf::from(".")); + assert!( + !executor.has_hooks_for_event(HookEvent::ToolCallBefore), + "globally-disabled hooks must report no fires even when one is configured" + ); + } + + #[test] + fn has_hooks_for_event_distinguishes_event_types() { + let config = HooksConfig { + enabled: true, + hooks: vec![ + Hook::new(HookEvent::SessionStart, "echo start"), + Hook::new(HookEvent::ToolCallBefore, "echo before"), + ], + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, PathBuf::from(".")); + // Configured events return true. + assert!(executor.has_hooks_for_event(HookEvent::SessionStart)); + assert!(executor.has_hooks_for_event(HookEvent::ToolCallBefore)); + // Unconfigured events return false even when other events are present. + assert!(!executor.has_hooks_for_event(HookEvent::ToolCallAfter)); + assert!(!executor.has_hooks_for_event(HookEvent::OnError)); + assert!(!executor.has_hooks_for_event(HookEvent::ModeChange)); + } } diff --git a/crates/tui/src/tui/tool_routing.rs b/crates/tui/src/tui/tool_routing.rs index 83efe2ba..27ae3b67 100644 --- a/crates/tui/src/tui/tool_routing.rs +++ b/crates/tui/src/tui/tool_routing.rs @@ -24,10 +24,10 @@ pub(super) fn handle_tool_call_started( // #455 (observer-only): fire `tool_call_before` hooks here, before // any UI bookkeeping. Hooks are read-only observers in this slice // — they can log, notify, or audit, but cannot mutate the args. - // Mutation support is tracked separately and stays a v0.8.9 - // follow-up because it requires a synchronous gate contract that - // doesn't exist today. - { + // Fast-path skip when no hooks are configured so per-tool + // dispatch doesn't pay for context construction in the common + // case (most users have no hooks). + if app.hooks.has_hooks_for_event(HookEvent::ToolCallBefore) { let context = app .base_hook_context() .with_tool_name(name) @@ -518,16 +518,20 @@ pub(super) fn handle_tool_call_complete( // result has settled. Hooks see tool_name + the result content // (or error message) + success flag. Read-only — they cannot // mutate the result that goes back to the model. Mutation - // remains a v0.8.9 follow-up. - let (result_text, success): (String, bool) = match result.as_ref() { - Ok(tool_result) => (tool_result.content.clone(), tool_result.success), - Err(err) => (err.to_string(), false), - }; - let context = app - .base_hook_context() - .with_tool_name(name) - .with_tool_result(&result_text, success, None); - let _ = app.execute_hooks(HookEvent::ToolCallAfter, &context); + // remains a v0.8.9 follow-up. Fast-path skip avoids the + // result.content.clone() and HookContext allocation when no + // hooks are configured. + if app.hooks.has_hooks_for_event(HookEvent::ToolCallAfter) { + let (result_text, success): (String, bool) = match result.as_ref() { + Ok(tool_result) => (tool_result.content.clone(), tool_result.success), + Err(err) => (err.to_string(), false), + }; + let context = app + .base_hook_context() + .with_tool_name(name) + .with_tool_result(&result_text, success, None); + let _ = app.execute_hooks(HookEvent::ToolCallAfter, &context); + } } /// Build a finalized standalone history cell for a tool completion whose diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index d3cceed7..ea2f0c00 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -2578,7 +2578,11 @@ pub(crate) fn apply_engine_error_to_app( // #455 (observer-only): fire `on_error` hooks so operators can // page on auth / billing / invalid-request failures without // tailing the audit log. Read-only — the hook can react but not - // suppress the error from reaching the transcript. + // suppress the error from reaching the transcript. Fast-path + // skip when no hooks configured. + if app + .hooks + .has_hooks_for_event(crate::hooks::HookEvent::OnError) { let context = app.base_hook_context().with_error(&message); let _ = app.execute_hooks(crate::hooks::HookEvent::OnError, &context); @@ -2894,6 +2898,10 @@ async fn dispatch_user_message( // dispatch. Hooks see the user's display text via the // `with_message` builder. Read-only — they can log, audit, or // notify but cannot mutate the message that goes to the engine. + // Fast-path skip when no hooks configured. + if app + .hooks + .has_hooks_for_event(crate::hooks::HookEvent::MessageSubmit) { let context = app.base_hook_context().with_message(&message.display); let _ = app.execute_hooks(crate::hooks::HookEvent::MessageSubmit, &context);