Repository navigation
perf(pm): TTY-gate + cooperative yield_now in install spawn loop #2910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b49c5d0
d3f049a
a7ae060
9dadc85
8c0657a
73f6335
72a7266
b0d015e
0f9c115
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -21,7 +21,8 @@ use crate::util::downloader::download_stats; | |||||
| use crate::util::json::load_package_lock_json_from_path; | ||||||
| use crate::util::linker::link; | ||||||
| use crate::util::logger::{ | ||||||
| PROGRESS_BAR, finish_progress_bar, log_progress, print_install_counts, start_progress_bar, | ||||||
| finish_progress_bar, log_progress, print_install_counts, progress_inc, progress_set_length, | ||||||
| start_progress_bar, | ||||||
| }; | ||||||
| use utoo_ruborist::compat::{is_cpu_compatible, is_os_compatible}; | ||||||
|
|
||||||
|
|
@@ -82,14 +83,34 @@ pub async fn install_packages( | |||||
| let mut depths: Vec<_> = groups.keys().cloned().collect(); | ||||||
| depths.sort_unstable(); | ||||||
|
|
||||||
| // Cooperative-yield cadence inside the synchronous spawn-loop body | ||||||
| // below. Before #2903 (TTY-gate), every iteration called | ||||||
| // `PROGRESS_BAR.inc(1)`, whose internal mutex+state update gave the | ||||||
| // tokio runtime roughly one micro-pause per package — enough for | ||||||
| // the runtime to interleave socket reads on in-flight tarball | ||||||
| // downloads. After TTY-gate the inc became a no-op on CI (non-TTY), | ||||||
| // and pcap+iostat A/B on the new disk-burst-bounded baseline showed | ||||||
| // utoo `tcp.analysis.zero_window` events climb from 2 → 16 vs | ||||||
| // baseline (no disk regression — util / w_await flat). Adding an | ||||||
| // explicit `yield_now` inside the spawn loop restores the implicit | ||||||
| // yield without the indicatif mutex cost. Every-N rather than | ||||||
| // per-iteration so the yield itself doesn't dominate cheap omit / | ||||||
| // skip iterations. | ||||||
| const YIELD_EVERY: usize = 32; | ||||||
|
|
||||||
| for depth in depths.iter() { | ||||||
| let mut clone_tasks: Vec<tokio::task::JoinHandle<Result<()>>> = Vec::new(); | ||||||
| let mut iter_count: usize = 0; | ||||||
|
|
||||||
| if let Some(packages) = groups.get(depth) { | ||||||
| for (path, package) in packages.iter() { | ||||||
| iter_count += 1; | ||||||
| if iter_count.is_multiple_of(YIELD_EVERY) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The is_multiple_of method is not available on primitive integer types like usize in the standard library (it was only stabilized for NonZero types in Rust 1.79). Unless a custom trait is being imported that provides this method, this will cause a compilation error. Using the remainder operator % is the standard and more portable way to perform this check.
Suggested change
|
||||||
| tokio::task::yield_now().await; | ||||||
| } | ||||||
| // Skip packages based on omit config | ||||||
| if should_omit_package(package, omit) { | ||||||
| PROGRESS_BAR.inc(1); | ||||||
| progress_inc(1); | ||||||
| continue; | ||||||
| } | ||||||
| let path = path.clone(); | ||||||
|
|
@@ -108,28 +129,28 @@ pub async fn install_packages( | |||||
| if package.link.is_some() { | ||||||
| let link_name = extract_package_name(&path); | ||||||
| if link_name.is_empty() { | ||||||
| PROGRESS_BAR.inc(1); | ||||||
| progress_inc(1); | ||||||
| continue; | ||||||
| } | ||||||
| link(Path::new(&resolved), Path::new(&path)) | ||||||
| .await | ||||||
| .with_context(|| format!("Link failed: {resolved} -> {path}"))?; | ||||||
| PROGRESS_BAR.inc(1); | ||||||
| progress_inc(1); | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| // skip when cpu or os is not compatible | ||||||
| if let Some(ref cpu) = package.cpu | ||||||
| && !is_cpu_compatible(cpu) | ||||||
| { | ||||||
| PROGRESS_BAR.inc(1); | ||||||
| progress_inc(1); | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| if let Some(ref os) = package.os | ||||||
| && !is_os_compatible(os) | ||||||
| { | ||||||
| PROGRESS_BAR.inc(1); | ||||||
| progress_inc(1); | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -153,18 +174,18 @@ pub async fn install_packages( | |||||
| tracing::warn!( | ||||||
| "Optional dependency {name} failed (ignored): {e:#}" | ||||||
| ); | ||||||
| PROGRESS_BAR.inc(1); | ||||||
| progress_inc(1); | ||||||
| return Ok(()); | ||||||
| } | ||||||
| return Err(e); | ||||||
| } | ||||||
| PROGRESS_BAR.inc(1); | ||||||
| progress_inc(1); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While progress_inc is now gated by IS_TTY, the log_progress call on the following line still executes a format! macro, which performs a string allocation even in non-TTY environments (like CI). To fully achieve the 'zero-cost' goal for CI mentioned in the PR description and the IS_TTY documentation, consider gating this call as well by checking IS_TTY before the allocation occurs. |
||||||
| log_progress(&format!("{name} resolved")); | ||||||
| update_package_binary(&target_path, &name).await | ||||||
| }); | ||||||
| clone_tasks.push(task); | ||||||
| } else { | ||||||
| PROGRESS_BAR.inc(1); | ||||||
| progress_inc(1); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -260,7 +281,7 @@ impl InstallService { | |||||
|
|
||||||
| if !package_lock.packages.is_empty() { | ||||||
| start_progress_bar(); | ||||||
| PROGRESS_BAR.set_length(package_lock.packages.len() as u64); | ||||||
| progress_set_length(package_lock.packages.len() as u64); | ||||||
| } | ||||||
|
|
||||||
| let link_start = Instant::now(); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -161,6 +161,27 @@ pub fn log_progress(text: &str) { | |||||
| PROGRESS_BAR.set_message(text.to_string()); | ||||||
| } | ||||||
|
|
||||||
| /// TTY-gated `PROGRESS_BAR.inc(1)`. Non-TTY callers pay no Mutex cost — | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The doc comment mentions inc(1), but the function signature accepts a generic n: u64. It is better to keep the documentation consistent with the implementation.
Suggested change
|
||||||
| /// indicatif's `inc` always takes the internal `Mutex<ProgressState>` | ||||||
| /// write-lock even with a hidden draw target, which contends across the | ||||||
| /// install loop's spawned tasks. | ||||||
| #[inline] | ||||||
| pub fn progress_inc(n: u64) { | ||||||
| if !*IS_TTY { | ||||||
| return; | ||||||
| } | ||||||
| PROGRESS_BAR.inc(n); | ||||||
| } | ||||||
|
|
||||||
| /// TTY-gated `PROGRESS_BAR.set_length`. Same rationale as [`progress_inc`]. | ||||||
| #[inline] | ||||||
| pub fn progress_set_length(len: u64) { | ||||||
| if !*IS_TTY { | ||||||
| return; | ||||||
| } | ||||||
| PROGRESS_BAR.set_length(len); | ||||||
| } | ||||||
|
|
||||||
| // Global timer for log_time/log_time_end | ||||||
| static START_TIME: OnceCell<Instant> = OnceCell::new(); | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The iter_count is currently reset for every depth level. In projects with many small dependency layers (e.g., a deep but narrow tree), this might prevent the cooperative yield from ever triggering, even if the total number of packages is large. Moving this counter outside the depths loop ensures a consistent yield cadence across the entire installation process, regardless of how packages are distributed across levels.