From e2aa86a9e8b6445488ef11b2c0a8b4c22a6f79ce Mon Sep 17 00:00:00 2001 From: Ogulcan Celik Date: Sun, 9 Aug 2026 20:36:11 +0300 Subject: [PATCH] test: guard pane-scaled render performance refs #2550 --- .../skills/herdr-pre-release-audit/SKILL.md | 3 +- .../references/pre-release-audit.md | 10 +- AGENTS.md | 27 ++- justfile | 18 +- scripts/test_ui_hot_path_architecture.py | 184 ++++++++++++++++++ src/pane.rs | 17 ++ src/pane/terminal.rs | 2 + src/server/render_stream.rs | 170 ++++++++++++++++ src/terminal/runtime.rs | 6 + src/ui/panes.rs | 2 + 10 files changed, 433 insertions(+), 6 deletions(-) create mode 100644 scripts/test_ui_hot_path_architecture.py diff --git a/.agents/skills/herdr-pre-release-audit/SKILL.md b/.agents/skills/herdr-pre-release-audit/SKILL.md index d3a24d46..585bd177 100644 --- a/.agents/skills/herdr-pre-release-audit/SKILL.md +++ b/.agents/skills/herdr-pre-release-audit/SKILL.md @@ -14,7 +14,8 @@ Read `references/pre-release-audit.md` and follow its workflow. Treat it as the - auditing `docs/next/CHANGELOG.md` - auditing `docs/next/README.md` and staged website docs - checking issue reference lines -- deciding when to run `just release-docs-check` +- deciding when to run `just pre-release-check` or its component checks +- running and assessing `just bench-render-scale` - producing the final release-readiness report Do not edit files during the audit unless the user explicitly asks to apply fixes. When applying fixes, keep changes scoped to the files named in the reference workflow. diff --git a/.agents/skills/herdr-pre-release-audit/references/pre-release-audit.md b/.agents/skills/herdr-pre-release-audit/references/pre-release-audit.md index 8666f644..c3e14c1b 100644 --- a/.agents/skills/herdr-pre-release-audit/references/pre-release-audit.md +++ b/.agents/skills/herdr-pre-release-audit/references/pre-release-audit.md @@ -75,10 +75,11 @@ Process: - `nix/package.nix` imports `Cargo.lock` through `cargoLock.lockFile`; normal version and lockfile updates do not require a separate cargo hash refresh. If git dependencies are introduced, verify the required `cargoLock.outputHashes` entries. - Run or recommend: ```bash - just release-docs-check + just pre-release-check ``` - - This check validates the staged draft, localized heading parity, published preview and stable snapshot provenance, and both production and draft website builds. - - Do not run `just release` unless the working tree is clean and the docs check passes. + - The docs check validates the staged draft, localized heading parity, published preview and stable snapshot provenance, and both production and draft website builds. + - The render benchmark has no automatic timing threshold, but reviewing it is a required release checkpoint. Record the 1, 15, and 50-count median and p95 results for background-workspace resize/layout and active panes, compare their scaling ratios, and treat a material regression as a release blocker until investigated rather than relying on absolute timing across machines. + - Do not run `just release` unless the working tree is clean, the docs check passes, and the render-scale result has been reviewed. 9. Apply changes only when asked. - Do not edit files during the audit unless the user explicitly asks you to apply fixes. @@ -118,6 +119,9 @@ Root docs finalized: YES | NO Nix Cargo lock integration: OK | NEEDS ATTENTION | NOT CHECKED +Render scaling: OK | NEEDS ATTENTION | NOT CHECKED +<1, 15, and 50-count median/p95 results and ratios for background-workspace resize/layout and active panes> + Required before release: 1. ``` diff --git a/AGENTS.md b/AGENTS.md index eeb260ee..8431529e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,6 +35,31 @@ These instructions are layered. - **Screen detection is evidence-based.** When changing `src/detect/manifests/`, first capture the relevant bottom-buffer state with `herdr agent read --source detection --format text` and, when styling or alternate screen behavior matters, `--format ansi`. Decide which visible controls are invariant, which are alternatives, and encode them as explicit AND/OR gates. Do not match whole-pane incidental text, and do not use the user-visible viewport for agent status because users can scroll it. - **UI patterns should be reused.** Herdr is a mouse-first TUI. New dialogs, onboarding, settings, and post-update flows should follow the existing UI/UX language and interaction patterns instead of inventing one-off screens. Prefer reusing existing modal/screen structure, affordances, and close actions so the app feels consistent. +### Multiplicative performance paths + +Treat work reachable from view computation, rendering, background-pane resizing, +PTY parsing, detection, and client frame fanout as multiplicative. Before adding +work, identify its frequency and cardinality: per byte, event, or render × panes, +tabs, or workspaces × attached clients. + +Inside pane-scaled render and layout loops: + +- Use narrow terminal-state accessors. Do not collect aggregate input state, + format terminal snapshots, inspect process trees, perform filesystem I/O, or + allocate when one scalar fact is enough. +- Keep terminal-core lock duration minimal. +- Preserve hidden-source and retained-render early exits. Hidden panes still + parse output, but their output must not trigger presentation work merely to + keep terminal or detection state current. +- When a change adds or widens work in one of these loops, profile fixed geometry + with 1 and at least 15 populated panes and report the scaling delta. Use + `just bench-render-scale` to exercise both background-workspace and active-pane + cardinality when applicable. + +Prefer deterministic operation or architecture tests to wall-clock CI limits. +Performance benchmarks are supporting evidence, not substitutes for behavioral +coverage. + ### Runtime/client boundary guardrail Herdr is migrating toward a server-owned runtime protocol with the TUI as one client. New work should not deepen the current server/TUI coupling. @@ -227,7 +252,7 @@ just check just release 0.x.y ``` -Before stable release, run `/pre-release-audit`, finalize `docs/next`, and let `just release-docs-check` validate the staged docs and website build. `just release` prepares the changelog and release commit, tags it, and pushes the tag. GitHub Actions builds binaries, creates the GitHub release, closes released issues, snapshots and promotes the tagged docs, and updates `website/latest.json`. +Before stable release, run `/pre-release-audit`, finalize `docs/next`, and run `just pre-release-check` to validate the staged docs, website build, and render scaling. `just release` prepares the changelog and release commit, tags it, and pushes the tag. GitHub Actions builds binaries, creates the GitHub release, closes released issues, snapshots and promotes the tagged docs, and updates `website/latest.json`. The release workflows must publish these four assets: diff --git a/justfile b/justfile index 4294d06f..f7a0e2e7 100644 --- a/justfile +++ b/justfile @@ -4,6 +4,7 @@ test: cargo nextest run --locked --status-level fail --final-status-level fail --failure-output final --success-output never python3 -m unittest scripts.test_agent_detection_manifest_check scripts.test_changelog scripts.test_config_reference_check scripts.test_docs_translation_parity scripts.test_hermes_integration_asset scripts.test_package_windows_conpty scripts.test_preview scripts.test_unix_installer scripts.test_vendor_libghostty_vt scripts.test_vendor_portable_pty + just ui-hot-path-architecture-test just integration-assets-test just plugin-marketplace-test @@ -11,6 +12,10 @@ test: test-one filter: cargo nextest run --locked "{{filter}}" --status-level fail --final-status-level fail --failure-output final --success-output never +# Enforce deterministic UI hot-path architecture boundaries +ui-hot-path-architecture-test: + python3 -m unittest scripts.test_ui_hot_path_architecture + # Run fast local lint checks [unix] lint: @@ -26,6 +31,7 @@ lint: [unix] ci filter='all()': lint cargo nextest run --locked -E "{{filter}}" --status-level fail --final-status-level slow --failure-output final --success-output never + just ui-hot-path-architecture-test just integration-assets-test just plugin-marketplace-test @@ -57,6 +63,10 @@ install-hooks: build: cargo build --release --locked +# Non-gating full-render scaling profile for background workspaces and active panes +bench-render-scale: + cargo test --release --locked --bin herdr render_scale_profile -- --ignored --nocapture --test-threads=1 + # Build the website and documentation website-build: cd website && bun install --frozen-lockfile && bun run build @@ -113,6 +123,12 @@ release-docs-check: just website-build cd website && bun run build:draft +# Validate release docs and review full-render scaling before release preparation +pre-release-check: + just release-docs-check + just bench-render-scale + @echo "release review required: investigate material render-scaling regressions before publishing." + # Prepare the release commit without tagging or pushing (usage: just release-prepare 0.1.1) release-prepare version: @printf '%s\n' '{{version}}' | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+$' || { \ @@ -128,7 +144,7 @@ release-prepare version: echo "error: tag v{{version}} already exists"; \ exit 1; \ fi - just release-docs-check + just pre-release-check python3 scripts/changelog.py prepare --version {{version}} cp CHANGELOG.md docs/next/CHANGELOG.md sed -i.bak 's/^version = ".*"/version = "{{version}}"/' Cargo.toml && rm -f Cargo.toml.bak diff --git a/scripts/test_ui_hot_path_architecture.py b/scripts/test_ui_hot_path_architecture.py new file mode 100644 index 00000000..44412197 --- /dev/null +++ b/scripts/test_ui_hot_path_architecture.py @@ -0,0 +1,184 @@ +from __future__ import annotations + +import re +import unittest +from pathlib import Path + + +PROJECT_ROOT = Path(__file__).resolve().parent.parent +HOT_PATH_SOURCES = ( + PROJECT_ROOT / "src" / "ui.rs", + *sorted((PROJECT_ROOT / "src" / "ui").rglob("*.rs")), + PROJECT_ROOT / "src" / "server" / "render_stream.rs", +) +TEST_MODULE = re.compile(r"(?m)^#\[cfg\(test\)\]\s*\nmod\s+\w+\s*\{") +FORBIDDEN_CALLS = ( + ( + re.compile(r"(?:\.|::)input_state\b"), + "aggregate terminal input state; add a narrow accessor", + ), + ( + re.compile(r"(?:\.|::)(?:keyboard_state_ansi|kitty_keyboard_state_ansi)\b"), + "formatted keyboard state", + ), + ( + re.compile(r"(?:\.|::)screen_text_snapshot\b"), + "formatted terminal screen snapshot", + ), + ( + re.compile(r"\bforeground_job\s*\("), + "process-tree inspection", + ), +) + + +def blank_non_newlines(chars: list[str], start: int, end: int) -> None: + for index in range(start, end): + if chars[index] != "\n": + chars[index] = " " + + +def mask_comments_and_literals(source: str) -> str: + chars = list(source) + index = 0 + while index < len(source): + if source.startswith("//", index): + end = source.find("\n", index + 2) + end = len(source) if end == -1 else end + blank_non_newlines(chars, index, end) + index = end + continue + + if source.startswith("/*", index): + depth = 1 + end = index + 2 + while end < len(source) and depth > 0: + if source.startswith("/*", end): + depth += 1 + end += 2 + elif source.startswith("*/", end): + depth -= 1 + end += 2 + else: + end += 1 + blank_non_newlines(chars, index, end) + index = end + continue + + if source[index] == "r": + quote = index + 1 + while quote < len(source) and source[quote] == "#": + quote += 1 + if quote < len(source) and source[quote] == '"': + suffix = '"' + "#" * (quote - index - 1) + end = source.find(suffix, quote + 1) + end = len(source) if end == -1 else end + len(suffix) + blank_non_newlines(chars, index, end) + index = end + continue + + if source[index] == '"': + end = index + 1 + while end < len(source): + if source[end] == "\\": + end += 2 + elif source[end] == '"': + end += 1 + break + else: + end += 1 + blank_non_newlines(chars, index, min(end, len(source))) + index = end + continue + + if source[index] == "'": + end = index + 2 + if index + 1 < len(source) and source[index + 1] == "\\": + end += 1 + if end < len(source) and source[end] == "'": + end += 1 + blank_non_newlines(chars, index, end) + index = end + continue + + index += 1 + + return "".join(chars) + + +def production_code(source: str) -> str: + code = mask_comments_and_literals(source) + chars = list(code) + search_from = 0 + + while test_module := TEST_MODULE.search(code, search_from): + depth = 0 + end = test_module.end() - 1 + while end < len(code): + if code[end] == "{": + depth += 1 + elif code[end] == "}": + depth -= 1 + if depth == 0: + end += 1 + break + end += 1 + blank_non_newlines(chars, test_module.start(), end) + code = "".join(chars) + search_from = end + + return code + + +class UiHotPathArchitectureTests(unittest.TestCase): + def test_render_hot_paths_avoid_known_expensive_runtime_queries(self) -> None: + violations: list[str] = [] + + for path in HOT_PATH_SOURCES: + source = path.read_text(encoding="utf-8") + code = production_code(source) + for pattern, description in FORBIDDEN_CALLS: + for match in pattern.finditer(code): + line = code.count("\n", 0, match.start()) + 1 + relative_path = path.relative_to(PROJECT_ROOT) + violations.append(f"{relative_path}:{line}: {description}") + + self.assertEqual( + violations, + [], + "Render/layout code must not perform pane-scaled expensive reads:\n" + + "\n".join(violations), + ) + + def test_scanner_ignores_non_production_references(self) -> None: + source = ''' +// runtime.input_state() +const EXAMPLE: &str = "runtime.input_state()"; +#[cfg(test)] +mod tests { + fn aggregate_state_test() { runtime.input_state(); } +} +fn production_after_tests() {} +''' + code = production_code(source) + self.assertNotRegex(code, FORBIDDEN_CALLS[0][0]) + self.assertIn("fn production_after_tests()", code) + self.assertEqual(code.count("\n"), source.count("\n")) + + def test_scanner_checks_production_after_test_modules(self) -> None: + source = ''' +#[cfg(test)] +mod tests { + const BRACES: &str = "}}"; +} +fn render() { TerminalRuntime::input_state; } +''' + self.assertRegex(production_code(source), FORBIDDEN_CALLS[0][0]) + + def test_scanner_catches_imported_process_query(self) -> None: + source = "fn render() { foreground_job(pid); }" + self.assertRegex(production_code(source), FORBIDDEN_CALLS[3][0]) + + +if __name__ == "__main__": + unittest.main() diff --git a/src/pane.rs b/src/pane.rs index b2c01f01..130c75e5 100644 --- a/src/pane.rs +++ b/src/pane.rs @@ -54,6 +54,21 @@ const RELEASE_REACQUIRE_SUPPRESSION: std::time::Duration = std::time::Duration:: const PANE_TERM: &str = "xterm-256color"; const PANE_COLORTERM: &str = "truecolor"; +#[cfg(test)] +thread_local! { + static AGGREGATE_INPUT_STATE_READS: Cell = const { Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_aggregate_input_state_reads() { + AGGREGATE_INPUT_STATE_READS.set(0); +} + +#[cfg(test)] +pub(crate) fn aggregate_input_state_reads() -> usize { + AGGREGATE_INPUT_STATE_READS.get() +} + fn apply_pane_terminal_env(cmd: &mut CommandBuilder) { // Each pane is rendered by herdr's own terminal layer, not the outer terminal // that launched the app. Advertising the inherited TERM leaks the host terminal @@ -2593,6 +2608,8 @@ impl PaneRuntime { } pub fn input_state(&self) -> Option { + #[cfg(test)] + AGGREGATE_INPUT_STATE_READS.set(AGGREGATE_INPUT_STATE_READS.get() + 1); self.terminal.input_state() } diff --git a/src/pane/terminal.rs b/src/pane/terminal.rs index 1c10726c..9575ad6f 100644 --- a/src/pane/terminal.rs +++ b/src/pane/terminal.rs @@ -1675,6 +1675,8 @@ impl GhosttyPaneTerminal { }) } + // This aggregate snapshot performs multiple terminal queries and may format + // keyboard state. Pane-scaled callers should add a narrow accessor instead. pub fn input_state(&self) -> Option { let Ok(core) = self.core.lock() else { return None; diff --git a/src/server/render_stream.rs b/src/server/render_stream.rs index f1f6c428..f14deb5e 100644 --- a/src/server/render_stream.rs +++ b/src/server/render_stream.rs @@ -467,3 +467,173 @@ fn focused_terminal_suppresses_host_cursor( .runtime_for_pane_in_workspace(terminal_runtimes, ws_idx, info.id) .is_some_and(crate::terminal::TerminalRuntime::synchronized_output_active) } + +#[cfg(test)] +mod render_scale_benchmark { + use std::hint::black_box; + use std::time::Instant; + + use ratatui::layout::Direction; + + use super::*; + use crate::app::Mode; + use crate::terminal::TerminalRuntime; + use crate::workspace::Workspace; + + const AREA: Rect = Rect::new(0, 0, 120, 40); + const SAMPLE_COUNT: usize = 40; + const WARMUP_COUNT: usize = 5; + + #[derive(Clone, Copy)] + struct RenderStats { + median_us: u128, + p95_us: u128, + max_us: u128, + } + + fn history() -> String { + (0..2_000).map(|line| format!("line-{line}\r\n")).collect() + } + + fn runtime(history: &str) -> TerminalRuntime { + TerminalRuntime::test_with_scrollback_bytes( + AREA.width, + AREA.height, + 1024 * 1024, + history.as_bytes(), + ) + } + + fn app_with_workspaces(workspace_count: usize) -> AppState { + let history = history(); + let workspaces = (0..workspace_count) + .map(|index| { + let mut workspace = Workspace::test_new(&format!("bench-{}", index + 1)); + let root_pane = workspace.tabs[0].root_pane; + workspace.tabs[0] + .runtimes + .insert(root_pane, runtime(&history)); + workspace + }) + .collect(); + app_with(workspaces) + } + + fn app_with_active_panes(pane_count: usize) -> AppState { + let history = history(); + let mut workspace = Workspace::test_new("bench"); + let root_pane = workspace.tabs[0].root_pane; + workspace.tabs[0] + .runtimes + .insert(root_pane, runtime(&history)); + let mut pane_ids = vec![root_pane]; + + for index in 1..pane_count { + let target = pane_ids[(index - 1) / 2]; + workspace.tabs[0].layout.focus_pane(target); + let direction = if index % 2 == 0 { + Direction::Vertical + } else { + Direction::Horizontal + }; + let pane_id = workspace.test_split(direction); + workspace.tabs[0] + .runtimes + .insert(pane_id, runtime(&history)); + pane_ids.push(pane_id); + } + + app_with(vec![workspace]) + } + + fn app_with(workspaces: Vec) -> AppState { + let mut app = AppState::test_new(); + app.mode = Mode::Terminal; + app.pane_scrollbars = true; + app.workspaces = workspaces; + app.active = Some(0); + app.selected = 0; + app + } + + fn profile(mut app: AppState) -> RenderStats { + for _ in 0..WARMUP_COUNT { + black_box(render_virtual(&mut app, AREA, true)); + } + + let mut samples = Vec::with_capacity(SAMPLE_COUNT); + for _ in 0..SAMPLE_COUNT { + let started = Instant::now(); + black_box(render_virtual(&mut app, AREA, true)); + samples.push(started.elapsed().as_micros()); + } + samples.sort_unstable(); + + RenderStats { + median_us: samples[SAMPLE_COUNT / 2], + p95_us: samples[(SAMPLE_COUNT - 1) * 95 / 100], + max_us: samples[SAMPLE_COUNT - 1], + } + } + + fn profile_cardinalities(build: fn(usize) -> AppState) -> [(usize, RenderStats); 3] { + [1, 15, 50].map(|count| (count, profile(build(count)))) + } + + fn print_profiles(label: &str, profiles: [(usize, RenderStats); 3]) { + let baseline_median_us = profiles[0].1.median_us as f64; + let baseline_p95_us = profiles[0].1.p95_us as f64; + println!("{label}"); + println!(" count median_us p95_us max_us median_vs_1x p95_vs_1x"); + for (count, stats) in profiles { + println!( + "{count:>10} {:>9} {:>6} {:>6} {:>12.2} {:>9.2}", + stats.median_us, + stats.p95_us, + stats.max_us, + stats.median_us as f64 / baseline_median_us, + stats.p95_us as f64 / baseline_p95_us, + ); + } + } + + fn assert_full_render_avoids_aggregate_input_state(mut app: AppState, scenario: &str) { + crate::pane::reset_aggregate_input_state_reads(); + black_box(render_virtual(&mut app, AREA, true)); + assert_eq!( + crate::pane::aggregate_input_state_reads(), + 0, + "full render collected aggregate input state for {scenario}", + ); + } + + #[tokio::test(flavor = "current_thread")] + async fn aggregate_input_state_counter_records_reads() { + let runtime = TerminalRuntime::test_with_screen_bytes(80, 24, b""); + crate::pane::reset_aggregate_input_state_reads(); + black_box(runtime.input_state()); + assert_eq!(crate::pane::aggregate_input_state_reads(), 1); + } + + #[tokio::test(flavor = "current_thread")] + async fn full_render_avoids_aggregate_input_state_reads() { + assert_full_render_avoids_aggregate_input_state( + app_with_workspaces(15), + "background workspaces", + ); + assert_full_render_avoids_aggregate_input_state(app_with_active_panes(15), "active panes"); + } + + #[tokio::test(flavor = "current_thread")] + #[ignore = "manual full-render scaling profile"] + async fn render_scale_profile() { + print_profiles( + "background-workspace resize/layout (one pane each)", + profile_cardinalities(app_with_workspaces), + ); + print_profiles( + "active panes (one workspace)", + profile_cardinalities(app_with_active_panes), + ); + } +} diff --git a/src/terminal/runtime.rs b/src/terminal/runtime.rs index d3900b68..89544a74 100644 --- a/src/terminal/runtime.rs +++ b/src/terminal/runtime.rs @@ -311,10 +311,16 @@ impl TerminalRuntime { self.0.word_motion_target(row, col, motion) } + /// Collects the complete terminal input-mode snapshot. + /// + /// This performs multiple terminal queries and may format keyboard state. + /// Keep it out of render/layout and pane-scaled loops; add a narrow accessor + /// when only one terminal fact is needed. pub fn input_state(&self) -> Option { self.0.input_state() } + /// Reads only whether the alternate screen is active. pub fn alternate_screen_active(&self) -> bool { self.0.alternate_screen_active() } diff --git a/src/ui/panes.rs b/src/ui/panes.rs index cce8ab02..f20b55b5 100644 --- a/src/ui/panes.rs +++ b/src/ui/panes.rs @@ -31,6 +31,8 @@ fn pane_border_title(label: &str, pane_width: u16, _focused: bool) -> Option Rect { if !pane_scrollbars || pane_inner.width <= 4 || rt.alternate_screen_active() { return pane_inner;