From 15636b64f832b58fc42eab4e0c4013eb72ba99eb Mon Sep 17 00:00:00 2001 From: elrrrrrrr Date: Thu, 30 Apr 2026 17:02:16 +0800 Subject: [PATCH] perf(pm): gate ProgressReceiver on IsTerminal (zero indicatif cost on CI) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A/B-verified perf fix isolated as its own PR. Detect stderr's TTY status once at startup (`IS_TTY: Lazy`) and short-circuit the entire `ProgressReceiver::on_event` path on non-TTY environments. ## Why this is needed indicatif's `inc()` / `set_message()` always acquire the internal `Mutex` write-lock and mutate `pos` / `est.buf` / `message`, regardless of TTY status — only the *rendering* short- circuits, not the state mutation. On the worker-pool resolve hot path that's 9000+ lock acquisitions per phase contending across multiple workers, costing measurable wall time on CI. ## A/B/A/A' bench evidence (CI Linux, ant-design / npmjs) | Round | indicatif config | Lock count | p1_resolve | σ | |---|---|---|---|---| | A | drop entirely | 0 | 2.50s | 0.10 | | B | full restore | 13713 | 2.73s | 0.07 | | A' | drop again (verify) | 0 | 2.58s | 0.15 | | A'' | only "resolved" event | 9142 | 2.65s | 0.05 | | this PR | TTY gate at startup | 0 in CI | 2.58s | 0.07 | A→B Δ = +0.23s (+9%); B→A Δ = −0.15s. Three independent runs across different GH Actions runners and Cloudflare egress conditions. Amortized cost is approximately linear in lock count (0.23 × 9142/13713 ≈ 0.15 ≈ predicted; observed 0.15). ## Why this approach - **Drop-only solutions** (A round) lose the local-dev progress UX. - **Partial drops** (A'' round, only-resolved) still pay 9142 locks. - **TTY gate** is the only solution that: - Pays zero overhead on CI / piped (`*IS_TTY = false` → match never enters) - Preserves full per-package "resolved {name}" UX on local dev (TTY) - Is a 1-file, 3-call-site change ## Counter-intuitive finding The first instinct ("CI has no TTY, indicatif must already short- circuit internally") is wrong: indicatif separates *rendering* (skipped on non-TTY / hidden draw target) from *state mutation* (always runs, always locks). Round A'' confirmed this empirically — 9142 locks with no rendering still cost +0.15s on p1_resolve. General rule: gate hot-path library calls at the call-site; never assume the library will short-circuit internally. ## Test plan - [x] `cargo build --release -p utoo-pm` clean - [x] `cargo clippy -p utoo-pm --all-targets -- -D warnings --no-deps` clean - [x] CI bench-phases-linux on PR #2818 round-10 (`f122478b`): p1_resolve 2.58s ±0.07 — matches "indicatif fully dropped" baseline (round-6/8) - [ ] Local dev TTY: `cargo run --profile release-local --bin utoo -- install` should show full per-package progress - [ ] Non-TTY: `… 2>/tmp/stderr.log` should produce zero indicatif ANSI sequences in the log --- crates/pm/src/util/logger.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/crates/pm/src/util/logger.rs b/crates/pm/src/util/logger.rs index 757a81070b..6ba124bb24 100644 --- a/crates/pm/src/util/logger.rs +++ b/crates/pm/src/util/logger.rs @@ -1,5 +1,6 @@ use anyhow::Result; use std::env; +use std::io::IsTerminal; use std::path::PathBuf; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; @@ -12,6 +13,22 @@ use tracing_subscriber::{ EnvFilter, Layer, Registry, fmt, layer::SubscriberExt, util::SubscriberInitExt, }; +/// Cached at startup: is stderr connected to a terminal? +/// +/// indicatif's `inc()` / `set_message()` always acquire the internal +/// `Mutex` write-lock and mutate `pos` / `est.buf` / +/// `message`, even when the draw target is hidden or the underlying +/// stream is not a TTY — only the *rendering* short-circuits, not the +/// state mutation. On the dependency-resolve hot path that's 9000+ +/// lock acquisitions per phase contending across multiple workers, +/// costing measurable wall time on CI. +/// +/// Gating every `ProgressReceiver` event behind this flag means +/// non-TTY environments (CI, piped output, `2>&1 | tee`) pay zero +/// indicatif cost — no `inc`, no `set_message`, no `format!()` +/// allocation. Local dev keeps the full progress UX. +pub static IS_TTY: Lazy = Lazy::new(|| std::io::stderr().is_terminal()); + pub static PROGRESS_BAR: Lazy = Lazy::new(|| { let pb = ProgressBar::new(0).with_style( ProgressStyle::with_template("{spinner:.blue} +{pos:.green} ~{len:.magenta} {wide_msg}") @@ -124,6 +141,9 @@ pub fn print_install_counts(added: usize, reused: usize, downloaded: usize) { } pub fn start_progress_bar() { + if !*IS_TTY { + return; + } PROGRESS_BAR.reset(); PROGRESS_BAR.set_style( ProgressStyle::with_template("{spinner:.blue} {pos:.green}/{len:.magenta} {wide_msg}") @@ -135,6 +155,9 @@ pub fn start_progress_bar() { } pub fn log_progress(text: &str) { + if !*IS_TTY { + return; + } PROGRESS_BAR.set_message(text.to_string()); } @@ -186,6 +209,14 @@ pub struct ProgressReceiver; impl utoo_ruborist::progress::EventReceiver for ProgressReceiver { fn on_event(&self, event: utoo_ruborist::progress::BuildEvent) { + // Single TTY check at the receiver boundary. Non-TTY (CI, piped + // output, `2>&1 | tee`) drops the entire indicatif update path — + // no Mutex write-lock, no format!() allocation, + // no String clones into set_message. Worth measurable wall time + // on the worker-pool resolve hot path; see `IS_TTY` doc comment. + if !*IS_TTY { + return; + } use utoo_ruborist::progress::BuildEvent; match event { BuildEvent::PreloadStart { count } | BuildEvent::PreloadQueued { count } => {