Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions crates/pm/src/util/logger.rs
Original file line number Diff line number Diff line change
@@ -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};

Expand All @@ -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<ProgressState>` 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<bool> = Lazy::new(|| std::io::stderr().is_terminal());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The IS_TTY check uses std::io::stderr().is_terminal(), which is appropriate for the progress bar. However, init_tracing (line 73) still uses the atty crate to check stdout. For consistency and to reduce dependencies, consider using std::io::stdout().is_terminal() in init_tracing as well, especially since IsTerminal is now available in the standard library (since Rust 1.70).


pub static PROGRESS_BAR: Lazy<ProgressBar> = Lazy::new(|| {
let pb = ProgressBar::new(0).with_style(
ProgressStyle::with_template("{spinner:.blue} +{pos:.green} ~{len:.magenta} {wide_msg}")
Expand Down Expand Up @@ -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}")
Expand All @@ -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());
}

Expand Down Expand Up @@ -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<ProgressState> 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;
}
Comment on lines +217 to +219

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While gating on_event significantly improves performance on the hot path, finish_progress_bar (line 107) still performs several indicatif operations (setting style, finishing with message, hiding draw target) and prints a terminal reset code (\x1b[0m) even in non-TTY environments. Although it checks for length, it's safer and more consistent to gate it with IS_TTY as well to ensure zero overhead and no stray ANSI codes in logs.

use utoo_ruborist::progress::BuildEvent;
match event {
BuildEvent::PreloadStart { count } | BuildEvent::PreloadQueued { count } => {
Expand Down
Loading