From 7945e91e30b91a379fcbbd68cf2100fe122205cd Mon Sep 17 00:00:00 2001 From: Eric Yue Date: Mon, 28 Sep 2026 03:34:03 +0800 Subject: [PATCH] perf: use keyed terminal lookup for named targets (#4669) --- justfile | 4 + src/app/terminal_targets.rs | 146 ++++++++++++++++++++++++++++++++++-- src/terminal/id.rs | 27 +++++++ 3 files changed, 171 insertions(+), 6 deletions(-) diff --git a/justfile b/justfile index 38a45052b..e3197c819 100644 --- a/justfile +++ b/justfile @@ -87,6 +87,10 @@ build: bench-render-scale: cargo test --release --locked --bin herdr render_scale_profile -- --ignored --nocapture --test-threads=1 +# Profile terminal target name resolution at increasing pane counts. +bench-terminal-targets: + cargo test --release --locked --bin herdr terminal_target_lookup_profile -- --ignored --nocapture --test-threads=1 + # ~3-5 minute CPU comparison; downloads stable unless HERDR_PERF_BASELINE_BIN is set bench-release-smoke: cargo build --release --locked diff --git a/src/app/terminal_targets.rs b/src/app/terminal_targets.rs index a8fe394a5..bbaaaf7dd 100644 --- a/src/app/terminal_targets.rs +++ b/src/app/terminal_targets.rs @@ -55,8 +55,7 @@ impl App { .filter(|candidate| { self.state .terminals - .values() - .find(|terminal| terminal.id.to_string() == candidate.terminal_id) + .get(candidate.terminal_id.as_str()) .is_some_and(|terminal| { terminal.agent_name.as_deref() == Some(target) || terminal.effective_agent_label() == Some(target) @@ -91,8 +90,7 @@ impl App { .filter(|candidate| { self.state .terminals - .values() - .find(|terminal| terminal.id.to_string() == candidate.terminal_id) + .get(candidate.terminal_id.as_str()) .is_some_and(|terminal| terminal.agent_name.as_deref() == Some(target)) }) .collect(); @@ -108,8 +106,7 @@ impl App { fn target_is_agent(&self, target: &TerminalTarget) -> bool { self.state .terminals - .values() - .find(|terminal| terminal.id.to_string() == target.terminal_id) + .get(target.terminal_id.as_str()) .is_some_and(|terminal| terminal.is_agent_terminal()) } @@ -193,3 +190,140 @@ impl App { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::app::{AppPolicy, AppState}; + + fn test_app() -> App { + let (_, api_rx) = tokio::sync::mpsc::unbounded_channel(); + App::new( + &crate::config::Config::default(), + AppPolicy::TEST, + None, + api_rx, + crate::api::EventHub::default(), + ) + } + + #[test] + fn named_targets_follow_terminal_identity_after_pane_and_tab_reordering() { + let mut app = test_app(); + app.state = AppState::test_with_adversarial_identity_state(); + let targets = app.terminal_targets(); + for (index, target) in targets.iter().enumerate() { + app.state + .terminals + .get_mut(target.terminal_id.as_str()) + .unwrap() + .set_agent_name(format!("worker-{index}")); + } + for (index, target) in targets.iter().enumerate() { + let name = format!("worker-{index}"); + assert_eq!(app.resolve_terminal_target(&name).unwrap(), *target); + assert_eq!(app.resolve_agent_target(&name).unwrap(), *target); + let pane = app.public_pane_id(target.ws_idx, target.pane_id).unwrap(); + assert_eq!(app.resolve_agent_target(&pane).unwrap(), *target); + } + app.state.assert_invariants_for_test(); + } + + #[test] + fn name_lookup_keeps_pane_order_and_ignores_detached_terminals() { + let mut app = test_app(); + app.state = AppState::test_with_adversarial_identity_state(); + let targets = app.terminal_targets(); + for target in &targets { + app.state + .terminals + .get_mut(target.terminal_id.as_str()) + .unwrap() + .set_agent_name("shared".into()); + } + let detached_id = crate::terminal::TerminalId::alloc(); + let mut detached = + crate::terminal::TerminalState::new(detached_id.clone(), std::env::temp_dir()); + detached.set_agent_name("detached".into()); + app.state.terminals.insert(detached_id, detached); + assert!(matches!( + app.resolve_agent_target("detached"), + Err(TerminalTargetError::NotFound { .. }) + )); + for result in [ + app.resolve_terminal_target("shared"), + app.resolve_agent_target("shared"), + ] { + let Err(TerminalTargetError::Ambiguous { candidates, .. }) = result else { + panic!("expected all attached panes to remain ambiguous"); + }; + assert_eq!( + candidates + .iter() + .map(|candidate| candidate.terminal_id.as_str()) + .collect::>(), + targets + .iter() + .map(|target| target.terminal_id.as_str()) + .collect::>(), + ); + } + } + + #[test] + #[ignore = "manual terminal target lookup scaling profile"] + fn terminal_target_lookup_profile() { + use std::hint::black_box; + use std::time::{Duration, Instant}; + + for count in [1, 15, 128, 512] { + let mut app = test_app(); + let mut workspace = crate::workspace::Workspace::test_new("lookup-profile"); + for _ in 1..count { + workspace.test_split(ratatui::layout::Direction::Horizontal); + } + app.state.workspaces = vec![workspace]; + app.state.ensure_test_terminals(); + let id = app.state.workspaces[0] + .terminal_id(app.state.workspaces[0].tabs[0].root_pane) + .unwrap() + .clone(); + app.state + .terminals + .get_mut(&id) + .unwrap() + .set_agent_name("profile-target".into()); + for (label, agent, target) in [ + ("terminal-name", false, "profile-target"), + ("agent-name", true, "profile-target"), + ("missing", false, "missing-target"), + ] { + let lookup = || { + if agent { + app.resolve_agent_target(target) + } else { + app.resolve_terminal_target(target) + } + }; + for _ in 0..32 { + black_box(lookup()).ok(); + } + let mut samples = Vec::new(); + for _ in 0..7 { + let start = Instant::now(); + let mut iterations = 0; + while start.elapsed() < Duration::from_millis(20) { + black_box(lookup()).ok(); + iterations += 1; + } + samples.push(start.elapsed().as_secs_f64() * 1e6 / f64::from(iterations)); + } + samples.sort_by(f64::total_cmp); + println!( + "terminal-target panes={count} case={label} median_us={:.3}", + samples[3] + ); + } + } + } +} diff --git a/src/terminal/id.rs b/src/terminal/id.rs index c7310753d..29a300a51 100644 --- a/src/terminal/id.rs +++ b/src/terminal/id.rs @@ -1,3 +1,4 @@ +use std::borrow::Borrow; use std::fmt; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{SystemTime, UNIX_EPOCH}; @@ -31,3 +32,29 @@ impl fmt::Display for TerminalId { f.write_str(&self.0) } } + +impl Borrow for TerminalId { + fn borrow(&self) -> &str { + self.as_str() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashMap; + + #[test] + fn terminal_ids_support_borrowed_lookup_without_changing_identity() { + let id = TerminalId::alloc(); + let serialized = serde_json::to_string(&id).unwrap(); + let restored: TerminalId = serde_json::from_str(&serialized).unwrap(); + let mut terminals = HashMap::from([(id.clone(), 1)]); + + assert_eq!(terminals.get(restored.as_str()), Some(&1)); + assert_eq!(terminals.get("missing-terminal"), None); + *terminals.get_mut(restored.as_str()).unwrap() = 2; + assert_eq!(terminals.remove(id.as_str()), Some(2)); + assert!(terminals.is_empty()); + } +}