fix(perf): eliminate redundant terminal wake work - #2962
Conversation
kangal-bot
added
the
ai-review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change replaces aggregate terminal-state reads with direct capability queries, adjusts PTY wakeup tracking for hidden and visible panes, adds architecture checks, and introduces a cross-platform release CPU smoke benchmark. ChangesRuntime behavior
Release performance validation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR changes terminal wake scheduling and adds release performance gating, but the current benchmark scripts can report success for an invalid zero-CPU comparison or an interrupted run, weakening the release check; duplicated pane-visibility logic can also drift and cause missed or extra wakeups. Merge readiness needs explicit owner follow-up on these bounded risks. Sequence Diagram(s)sequenceDiagram
participant AppState
participant RenderSignal
participant HeadlessObserver
AppState->>RenderSignal: Set immediate application-surface pane IDs
HeadlessObserver->>RenderSignal: Request observed and hidden PTY sources
RenderSignal-->>HeadlessObserver: Report immediate presentation work
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
scripts/release_perf_case.sh (4)
66-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
root_paneandworkspace_idlike the loop validatespane_id.
jq -rprints the stringnullwhen the field is missing. Line 74 checks for that case, but lines 66 and 67 do not. A malformed API response makes the failure surface later as a confusingpane runortab createerror.♻️ Proposed change
root_pane=$(printf '%s\n' "$panes_json" | jq -r '.result.panes[0].pane_id') workspace_id=$("${control_env[@]}" "$bin" workspace list | jq -r '.result.workspaces[0].workspace_id') +[[ -n "$root_pane" && "$root_pane" != null ]] || { echo "session did not report a root pane" >&2; exit 1; } +[[ -n "$workspace_id" && "$workspace_id" != null ]] || { echo "session did not report a workspace" >&2; exit 1; }
101-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilence the two shellcheck findings with explicit directives.
Both hints are false positives here. Line 102 relies on deliberate word splitting of
$all_pids, and the comma on line 105 belongs to the-statsvalue oftop. Add directives so the lint output stays clean and the intent is documented.♻️ Proposed change
if [[ $platform == linux ]]; then + # shellcheck disable=SC2086 # intentional word splitting of the pid list pid_csv=$(printf '%s\n' $all_pids | paste -sd, -) LC_ALL=C pidstat -h -u -p "$pid_csv" 1 "$seconds" > "$raw" else + # shellcheck disable=SC2054 # the comma is part of the top -stats value top_args=(top -l $((seconds + 1)) -s 1 -stats pid,cpu,time -n 2)Source: Linters/SAST tools
110-117: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMatch the pid in the PID column instead of anywhere in the row.
mean_linuxscans every field fortarget. Another numeric column, for example UID or the CPU-core index, can equal the pid value and cause a wrong row to be attributed. Withpidstat -h, the pid is always the third field.The sample-count assertion on line 131 catches most of these cases, so this is robustness only.
♻️ Proposed change
awk -v target="$2" ' /^Linux/ || /^`#/` || NF < 5 { next } - { found=0; for (i=1; i<=NF; i++) if ($i == target) { found=1; break } - if (found && $(NF-2) ~ /^[0-9]+([.][0-9]+)?$/) { sum += $(NF-2); count++ } } + $3 == target && $(NF-2) ~ /^[0-9]+([.][0-9]+)?$/ { sum += $(NF-2); count++ } END { if (!count) exit 1; printf "%.6f,%d", sum/count, count }
41-53: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInterrupted runs exit with status 0 in both benchmark scripts. Both scripts bind one
cleanupfunction toEXIT INT TERM. On INT or TERM the handler returns afterrm -rf, and the script exits with the status of the last command incleanup, which is normally 0. An interrupted benchmark then looks like a successful one to the caller and tojust pre-release-check.
scripts/release_perf_case.sh#L41-L53: keeptrap cleanup EXITand addtrap 'cleanup; exit 130' INTandtrap 'cleanup; exit 143' TERM.scripts/release_perf_smoke.sh#L27-L28: apply the same split so an interrupted smoke run reports failure instead of printing no verdict and exiting 0.scripts/release_perf_producer.pl (2)
11-20: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winClamp the schedule when the producer falls behind.
$nextadvances by exactly$periodon every iteration. If the process is descheduled or the write blocks,$remainingbecomes negative and the loop emits lines with no sleep until it catches up. The producer then competes for CPU with the process under measurement and distorts the benchmark result, which is the value the release gate compares.Reset the schedule when the deficit exceeds one period.
♻️ Proposed change
while (1) { $sequence++; printf "\rbench-output-%08d-%s", $sequence, $label; $next += $period; - my $remaining = $next - clock_gettime(CLOCK_MONOTONIC); - sleep $remaining if $remaining > 0; + my $now = clock_gettime(CLOCK_MONOTONIC); + my $remaining = $next - $now; + if ($remaining > 0) { + sleep $remaining; + } elsif ($remaining < -$period) { + $next = $now + $period; + } }
6-9: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueValidate that
$rateis a positive number.Line 11 computes
1 / $rate. A non-numeric or zero$rateproduces a division-by-zero fatal error or a warning-laden period. The current guard only rejects falsy values.♻️ Proposed change
my ($rate, $gate, $label) = `@ARGV`; -die "usage: $0 <rate-hz> <gate-file> <label>\n" unless $rate && $gate && $label; +die "usage: $0 <rate-hz> <gate-file> <label>\n" + unless defined $rate && defined $gate && defined $label && length $label; +die "rate must be a positive number\n" unless $rate =~ /^[0-9]*\.?[0-9]+$/ && $rate > 0;scripts/release_perf_smoke.sh (2)
31-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve
website/latest.jsonrelative to the repository, not the current directory.Line 33 reads
website/latest.jsonfrom the current working directory.just bench-release-smokeruns from the repository root, so that path works. A direct invocation from any other directory fails with ajqerror that does not name the missing file.script_diris already available in this script; compute it before this block and derive the path from it.♻️ Proposed change
+script_dir=$(cd "$(dirname "$0")" && pwd) +repo_root=$(cd "$script_dir/.." && pwd) + baseline=${HERDR_PERF_BASELINE_BIN:-} if [[ -z "$baseline" ]]; then - baseline_version=$(jq -er '.version' website/latest.json) + baseline_version=$(jq -er '.version' "$repo_root/website/latest.json")Then drop the duplicate
script_dirassignment on line 44.
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck for
curlonly when the script downloads the baseline.Line 11 requires
curlon every run. WhenHERDR_PERF_BASELINE_BINpoints at a local binary, the script never callscurl. This blocks offline runs with a local baseline for no reason.justfile (1)
142-142: 📐 Maintainability & Code Quality | 🔵 TrivialNote the added wall-clock cost and the network dependency of
pre-release-check.
bench-release-smokeruns 8 benchmark cases. Each case creates up to 50 panes, waitsHERDR_PERF_WARMUP_SECONDS, and samples forHERDR_PERF_SAMPLE_SECONDS. It also downloads the current stable binary from GitHub unlessHERDR_PERF_BASELINE_BINis set.pre-release-checktherefore now needs network access and takes several minutes longer.Consider documenting the expected duration and the
HERDR_PERF_BASELINE_BINescape hatch next to this target.AGENTS.md (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the "longer release matrix" so the instruction is actionable.
The new text tells the reader when to use "the longer release matrix" but does not say what it is. The surrounding section names concrete commands, for example
just bench-render-scale. Reference the specific command or document for the longer matrix.src/server/headless.rs (1)
4090-4136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the two independent pane-visibility implementations.
sync_immediate_pty_sourcesrebuilds pane visibility fromapp_surface_pane_ids()plus a manual scan of every workspace/tab for direct-terminal-target matches.pty_source_visible_to_render_targets(used later at line 751 to classify the render plan) computes the same "is this pane visible to a connected render target" rule per pane, usingterminal_id_for_paneandapp_surface_contains_pane.Both derive from
pty_render_targets(), but they express the visibility rule in two separate places. They agree today, but a future change to one rule (for example, a new client mode, or a change to zoom/popup semantics) can silently desynchronize the other, causing either missed wakes for genuinely visible panes or unnecessary wakes for hidden ones.Build the
immediate_pty_sourcesset by iterating candidate panes and calling the existing per-panepty_source_visible_to_render_targetscheck (or extract a single shared helper that both call sites use), instead of maintaining two separate reconstructions of the same rule.♻️ Illustrative direction (not a drop-in patch)
fn sync_immediate_pty_sources(&self) { let (has_app_target, direct_terminal_targets) = self.pty_render_targets(); - let mut pane_ids = if has_app_target { - self.app.state.app_surface_pane_ids() - } else { - HashSet::new() - }; - if !direct_terminal_targets.is_empty() { - for workspace in &self.app.state.workspaces { - for tab in &workspace.tabs { - pane_ids.extend(tab.panes.iter().filter_map(|(&pane_id, pane)| { - direct_terminal_targets - .contains(pane.attached_terminal_id.as_str()) - .then_some(pane_id) - })); - } - } - if let Some(popup) = &self.app.state.popup_pane { - if direct_terminal_targets.contains(popup.terminal_id.as_str()) { - pane_ids.insert(popup.pane_id); - } - } - } + let pane_ids = self + .all_known_pane_ids() + .filter(|&pane_id| { + self.pty_source_visible_to_render_targets(pane_id, has_app_target, &direct_terminal_targets) + }) + .collect(); self.app.render_dirty.set_immediate_pty_sources(pane_ids); }Also consider adding a test that connects both an
Appclient and aTerminalAttach/TerminalObserveclient at once, to exercise the combinedhas_app_target && !direct_terminal_targets.is_empty()branch, which no current test covers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af4bebeb-5035-44e9-8ca4-130f76448178
📒 Files selected for processing (28)
AGENTS.mdCHANGELOG.mddocs/next/CHANGELOG.mdjustfilescripts/release_perf_case.shscripts/release_perf_producer.plscripts/release_perf_smoke.shscripts/test_ui_hot_path_architecture.pysrc/app/actions.rssrc/app/api_helpers.rssrc/app/input/mouse.rssrc/app/input/terminal.rssrc/app/mod.rssrc/app/state.rssrc/ghostty/bindings.rssrc/ghostty/mod.rssrc/input/mod.rssrc/input/model.rssrc/pane.rssrc/pane/terminal.rssrc/render_signal.rssrc/server/headless.rssrc/server/terminal_attach.rssrc/terminal/runtime.rsvendor/libghostty-vt.patches.mdvendor/libghostty-vt/include/ghostty/vt/terminal.hvendor/libghostty-vt/src/terminal/c/terminal.zigvendor/patches/libghostty-vt/0002-expose-modify-other-keys-mode.patch
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
|
Addressed the benchmark review in d78bf69: interrupt failures, API ID validation, stable PID parsing, producer rate/catch-up validation, local-baseline/off-root support, benchmark sanity checks, and concrete release guidance. I also added coverage for the combined app + direct terminal observer path. I kept the two visibility paths separate intentionally: synchronization materializes known immediate pane sources using the active-surface fast path, while request classification must fail open for stale/unknown pane IDs. Iterating all panes through request classification on every synchronization would widen a pane-scaled hot path. |
Greptile SummaryThe PR reduces redundant terminal wake work and replaces aggregate terminal input-state reads with narrow mode queries.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/render_signal.rs | Introduces visibility-aware PTY wake coalescing while preserving generic and terminal-title presentation work. |
| src/server/headless.rs | Integrates immediate PTY-source tracking with app clients and direct terminal observers in the headless render loop. |
| src/app/state.rs | Computes the popup and active-tab pane set currently presented by the app surface. |
| src/pane/terminal.rs | Adds narrow terminal-mode queries and replaces aggregate input-state reads on hot input paths. |
| src/ghostty/mod.rs | Exposes the vendored modifyOtherKeys state through the Rust Ghostty wrapper. |
| vendor/libghostty-vt/src/terminal/c/terminal.zig | Adds the underlying terminal-data query for modifyOtherKeys mode. |
| scripts/release_perf_smoke.sh | Orchestrates stable-versus-candidate CPU comparisons for visible and hidden output scenarios. |
| scripts/release_perf_case.sh | Creates isolated benchmark sessions, gathers process CPU samples, and cleans up benchmark resources. |
| scripts/test_ui_hot_path_architecture.py | Extends deterministic enforcement of narrow terminal-state access on multiplicative UI paths. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
PTY[PTY output] --> RS[RenderSignal coalescing]
RS -->|visible pane or direct observer| IW[Immediate wake]
RS -->|hidden pane only| CD[Cadence-deferred work]
IW --> LOOP[App or headless server loop]
CD --> LOOP
LOOP --> FRAME[Render and stream frame]
TERM[Ghostty terminal state] --> SQ[Scalar mode queries]
SQ --> INPUT[Keyboard, paste, focus, mouse routing]
Reviews (2): Last reviewed commit: "test(perf): strengthen performance guard..." | Re-trigger Greptile
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/test_ui_hot_path_architecture.py (1)
19-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct coverage for each aggregate-state rule.
The scanner tests use positional
FORBIDDEN_CALLSindexes. They do not directly testkeyboard_state_ansiorkitty_keyboard_state_ansi. A rule reorder or regex regression could leave these new guardrails untested. Add fixtures for each new pattern, or expose named rule constants instead of relying on tuple indexes.src/server/headless.rs (1)
9787-9817: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert immediate presentation work for a direct terminal observer.
The test proves that the observed pane is visible to a render target. It does not prove that
sync_immediate_pty_sources()made the PTY request immediate. A regression inset_immediate_pty_sources()could therefore pass this test.Add an assertion after requesting
background_pane:Proposed test strengthening
let hidden_pane = server.app.state.workspaces[0].tabs[0].root_pane; server.sync_immediate_pty_sources(); - assert!(server.app.render_dirty.request_pty(hidden_pane)); assert!(server.app.render_dirty.request_pty(background_pane)); + assert!(server.has_pending_presentation_work(false, false)); + assert!(server.app.render_dirty.request_pty(hidden_pane));
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 290586d3-026a-4a61-b9bc-f960a146a3ff
📒 Files selected for processing (7)
AGENTS.mdjustfilescripts/release_perf_case.shscripts/release_perf_producer.plscripts/release_perf_smoke.shscripts/test_ui_hot_path_architecture.pysrc/server/headless.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- justfile
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
|
Addressed the final CodeRabbit test suggestions in 33addf6: every aggregate-state scanner rule now has direct coverage, and the mixed app + terminal observer test proves that the observed background source itself creates immediate presentation work. |
kangal-bot
removed
the
ai-review
* fix(perf): eliminate redundant terminal wake work * fix(perf): harden release benchmark gate * test(perf): strengthen performance guardrails
2 participants
Summary
Performance
Validation
just checkjust pre-release-checkjust bench-release-smokeghostty_modify_other_keys_mode_one_preserves_shift_enterruntime test