test: support -N, -G and -O on Windows - #13658
Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
GNU testsuite comparison: |
|
@codspeedbot explain why this is faster (and this test unstable) |
002dca7 to
b9f7518
Compare
Short answer: it isn't actually faster — the base run is the outlier, and this Why the report shows +3.28%This PR touches only
The instruction count — the part that actually reflects the code — is flat. The whole delta sits in the modeled cache/memory cost and in the number of syscalls the run performed. Looking at the last few
So the head value is right on the usual baseline; the base commit happened to land 3% high. Note also the environment warning in the report: the Why this benchmark is unstableThe flamegraph shows where the time goes: That mix is the worst case for reproducibility:
This is a known trait of this family: If you want to fix itHappy to open a follow-up PR (this one isn't the right place) doing one of:
Just say which you prefer and I'll take care of it. Summary
Learnings updated. See what CodSpeed remembers about this repository. Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot. |
|
@codspeedbot yes, fix it in a new pr |
I dug into it, and the instability is not the filesystem or the cache model — it comes from Root cause
Counting syscalls by type over two runs of the same binary on the same tree (5,460 dirs, 13,650 files) — everything is deterministic except the futexes:
At most one extra syscall per directory — which is exactly what CodSpeed recorded between two runs of Fix1. 2. Benchmarks: stop measuring the teardown of the temporary tree. The Results (simulation, 5 consecutive local runs of the whole
|
| Benchmark | Before | After | Change |
|---|---|---|---|
du_summarize_balanced_tree[(5, 4, 10)] |
6.17 ms | 5.77 ms | −6.5% |
du_wide_tree[(5000, 500)] |
9.15 ms | 7.82 ms | −14.5% |
du_all_wide_tree[(5000, 500)] |
16.69 ms | 13.38 ms | −19.8% |
du_deep_tree[(100, 3)] |
1.06 ms | 0.89 ms | −16.0% |
du_max_depth_balanced_tree[(6, 4, 10)] |
23.92 ms | 22.35 ms | −6.6% |
Part of the gain on the wide/deep benchmarks is the teardown no longer being measured; the rest, and all of the gain on the balanced ones, is the reduced futex traffic.
Validation
- Output compared byte-for-byte with the pre-change binary for
-a,-s,-c,-h,-b,-x,-L,--inodes,--time,--separate-dirs,--threshold,--max-depth, and error paths — identical;--max-depthand-aoutput also matches GNUdu. cargo test -p coreutils --test tests -- test_du: 112 passed.test_du_inaccessible_directoryfails with and without the change in my sandbox because the tests run as root.cargo fmtandcargo clippy -p uu_du --all-targetsclean.
The three benchmarks commented out with /* too much variance */ are rewritten on top of the new helper but left disabled — worth re-enabling once a few main runs confirm the stability of the active ones.
Patch (git apply)
diff --git a/src/uu/du/benches/du_bench.rs b/src/uu/du/benches/du_bench.rs
index 0b63ce9..25437e0 100644
--- a/src/uu/du/benches/du_bench.rs
+++ b/src/uu/du/benches/du_bench.rs
@@ -3,20 +3,32 @@
// For the full copyright and license information, please view the LICENSE
// file that was distributed with this source code.
+use std::path::Path;
+
use divan::{Bencher, black_box};
use tempfile::TempDir;
use uu_du::uumain;
use uucore::benchmark::{fs_tree, run_util_function};
-/// Helper to run du with given arguments on a directory
-fn bench_du_with_args(bencher: Bencher, temp_dir: &TempDir, args: &[&str]) {
- let temp_path_str = temp_dir.path().to_str().unwrap();
- let mut full_args = args.to_vec();
- full_args.push(temp_path_str);
-
- bencher.bench(|| {
- black_box(run_util_function(uumain, &full_args));
- });
+/// Helper to run du with given arguments on a freshly created directory tree
+///
+/// The tree is built in the input phase (`with_inputs`) and handed to the timed
+/// closure by reference (`bench_refs`), so that both the creation *and* the
+/// deletion of the temporary tree stay out of the measured region. Passing the
+/// tree by value would make each iteration pay for the recursive removal of tens
+/// of thousands of entries, which has nothing to do with what du does.
+fn bench_du(bencher: Bencher, build_tree: impl Fn(&Path) + Sync, args: &[&str]) {
+ bencher
+ .with_inputs(|| {
+ let temp_dir = TempDir::new().unwrap();
+ build_tree(temp_dir.path());
+ temp_dir
+ })
+ .bench_refs(|temp_dir| {
+ let mut full_args = args.to_vec();
+ full_args.push(temp_dir.path().to_str().unwrap());
+ black_box(run_util_function(uumain, &full_args));
+ });
}
/* too much variance
@@ -26,9 +38,11 @@ fn du_balanced_tree(
bencher: Bencher,
(depth, dirs_per_level, files_per_dir): (usize, usize, usize),
) {
- let temp_dir = TempDir::new().unwrap();
- fs_tree::create_balanced_tree(temp_dir.path(), depth, dirs_per_level, files_per_dir);
- bench_du_with_args(bencher, &temp_dir, &[]);
+ bench_du(
+ bencher,
+ |path| fs_tree::create_balanced_tree(path, depth, dirs_per_level, files_per_dir),
+ &[],
+ );
}
*/
@@ -39,9 +53,11 @@ fn du_all_balanced_tree(
bencher: Bencher,
(depth, dirs_per_level, files_per_dir): (usize, usize, usize),
) {
- let temp_dir = TempDir::new().unwrap();
- fs_tree::create_balanced_tree(temp_dir.path(), depth, dirs_per_level, files_per_dir);
- bench_du_with_args(bencher, &temp_dir, &["-a"]);
+ bench_du(
+ bencher,
+ |path| fs_tree::create_balanced_tree(path, depth, dirs_per_level, files_per_dir),
+ &["-a"],
+ );
}
*/
@@ -52,58 +68,42 @@ fn du_human_balanced_tree(
bencher: Bencher,
(depth, dirs_per_level, files_per_dir): (usize, usize, usize),
) {
- let temp_dir = TempDir::new().unwrap();
- fs_tree::create_balanced_tree(temp_dir.path(), depth, dirs_per_level, files_per_dir);
- bench_du_with_args(bencher, &temp_dir, &["-h"]);
+ bench_du(
+ bencher,
+ |path| fs_tree::create_balanced_tree(path, depth, dirs_per_level, files_per_dir),
+ &["-h"],
+ );
}
*/
/// Benchmark du on wide directory structures (many files/dirs, shallow)
#[divan::bench(args = [(5000, 500)])]
fn du_wide_tree(bencher: Bencher, (total_files, total_dirs): (usize, usize)) {
- bencher
- .with_inputs(|| {
- let temp_dir = TempDir::new().unwrap();
- fs_tree::create_wide_tree(temp_dir.path(), total_files, total_dirs);
- temp_dir
- })
- .bench_values(|temp_dir| {
- let temp_path_str = temp_dir.path().to_str().unwrap();
- let args = vec![temp_path_str];
- black_box(run_util_function(uumain, &args));
- });
+ bench_du(
+ bencher,
+ |path| fs_tree::create_wide_tree(path, total_files, total_dirs),
+ &[],
+ );
}
/// Benchmark du -a on wide directory structures
#[divan::bench(args = [(5000, 500)])]
fn du_all_wide_tree(bencher: Bencher, (total_files, total_dirs): (usize, usize)) {
- bencher
- .with_inputs(|| {
- let temp_dir = TempDir::new().unwrap();
- fs_tree::create_wide_tree(temp_dir.path(), total_files, total_dirs);
- temp_dir
- })
- .bench_values(|temp_dir| {
- let temp_path_str = temp_dir.path().to_str().unwrap();
- let args = vec![temp_path_str, "-a"];
- black_box(run_util_function(uumain, &args));
- });
+ bench_du(
+ bencher,
+ |path| fs_tree::create_wide_tree(path, total_files, total_dirs),
+ &["-a"],
+ );
}
/// Benchmark du on deep directory structures
#[divan::bench(args = [(100, 3)])]
fn du_deep_tree(bencher: Bencher, (depth, files_per_level): (usize, usize)) {
- bencher
- .with_inputs(|| {
- let temp_dir = TempDir::new().unwrap();
- fs_tree::create_deep_tree(temp_dir.path(), depth, files_per_level);
- temp_dir
- })
- .bench_values(|temp_dir| {
- let temp_path_str = temp_dir.path().to_str().unwrap();
- let args = vec![temp_path_str];
- black_box(run_util_function(uumain, &args));
- });
+ bench_du(
+ bencher,
+ |path| fs_tree::create_deep_tree(path, depth, files_per_level),
+ &[],
+ );
}
/// Benchmark du -s (summarize) on balanced tree
@@ -112,9 +112,11 @@ fn du_summarize_balanced_tree(
bencher: Bencher,
(depth, dirs_per_level, files_per_dir): (usize, usize, usize),
) {
- let temp_dir = TempDir::new().unwrap();
- fs_tree::create_balanced_tree(temp_dir.path(), depth, dirs_per_level, files_per_dir);
- bench_du_with_args(bencher, &temp_dir, &["-s"]);
+ bench_du(
+ bencher,
+ |path| fs_tree::create_balanced_tree(path, depth, dirs_per_level, files_per_dir),
+ &["-s"],
+ );
}
/// Benchmark du with --max-depth
@@ -123,9 +125,11 @@ fn du_max_depth_balanced_tree(
bencher: Bencher,
(depth, dirs_per_level, files_per_dir): (usize, usize, usize),
) {
- let temp_dir = TempDir::new().unwrap();
- fs_tree::create_balanced_tree(temp_dir.path(), depth, dirs_per_level, files_per_dir);
- bench_du_with_args(bencher, &temp_dir, &["--max-depth=2"]);
+ bench_du(
+ bencher,
+ |path| fs_tree::create_balanced_tree(path, depth, dirs_per_level, files_per_dir),
+ &["--max-depth=2"],
+ );
}
fn main() {
diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs
index f30127e..2c25510 100644
--- a/src/uu/du/src/du.rs
+++ b/src/uu/du/src/du.rs
@@ -3,11 +3,12 @@
// For the full copyright and license information, please view the LICENSE
// file that was distributed with this source code.
//
-// spell-checker:ignore fstatat openat dirfd
+// spell-checker:ignore fstatat openat dirfd futex
use clap::{Arg, ArgAction, ArgMatches, Command, builder::PossibleValue};
use glob::{Pattern, PatternError};
use rustc_hash::FxHashSet as HashSet;
+use std::cell::RefCell;
use std::env;
use std::ffi::{OsStr, OsString};
use std::fs::{self, DirEntry, File, Metadata};
@@ -335,10 +336,10 @@ fn safe_du(
options: &TraversalOptions,
depth: usize,
seen_inodes: &mut HashSet<FileInfo>,
- print_tx: &mpsc::Sender<UResult<StatPrintInfo>>,
+ print_tx: &StatPrinterTx,
parent_fd: Option<&DirFd>,
initial_stat: Option<std::io::Result<Stat>>,
-) -> Result<Stat, Box<mpsc::SendError<UResult<StatPrintInfo>>>> {
+) -> Result<Stat, Box<SendError>> {
// The caller provides an already-computed stat for this entry, which lets us
// avoid re-stating it here. For subdirectories this saves both a redundant
// `fstatat` and an expensive full-path `lstat` per directory (the dominant cost
@@ -368,9 +369,7 @@ fn safe_du(
let error = e.map_err_context(
|| translate!("du-error-cannot-access", "path" => path.quote()),
);
- if let Err(send_error) = print_tx.send(Err(error)) {
- return Err(Box::new(send_error));
- }
+ print_tx.send(Err(error))?;
return Err(Box::new(mpsc::SendError(Err(USimpleError::new(
0,
"Error already handled",
@@ -381,9 +380,7 @@ fn safe_du(
let error = e.map_err_context(
|| translate!("du-error-cannot-access", "path" => path.quote()),
);
- if let Err(send_error) = print_tx.send(Err(error)) {
- return Err(Box::new(send_error));
- }
+ print_tx.send(Err(error))?;
return Err(Box::new(mpsc::SendError(Err(USimpleError::new(
0,
"Error already handled",
@@ -592,10 +589,10 @@ fn du_regular(
options: &TraversalOptions,
depth: usize,
seen_inodes: &mut HashSet<FileInfo>,
- print_tx: &mpsc::Sender<UResult<StatPrintInfo>>,
+ print_tx: &StatPrinterTx,
ancestors: Option<&mut HashSet<FileInfo>>,
symlink_depth: Option<usize>,
-) -> Result<Stat, Box<mpsc::SendError<UResult<StatPrintInfo>>>> {
+) -> Result<Stat, Box<SendError>> {
// Maximum symlink depth to prevent infinite loops
const MAX_SYMLINK_DEPTH: usize = 40;
@@ -827,6 +824,76 @@ struct StatPrintInfo {
depth: usize,
}
+/// Error returned when a message cannot be handed over to the printing thread.
+type SendError = mpsc::SendError<UResult<StatPrintInfo>>;
+
+/// Number of messages accumulated before they are handed over to the printing thread.
+const PRINT_BATCH_SIZE: usize = 16;
+
+/// Buffered sender towards the printing thread.
+///
+/// The traversal produces one message per directory (and one per file with `-a`).
+/// Sending them one at a time wakes the printing thread up for nearly every
+/// message, which costs a futex syscall per directory and makes the traversal
+/// sensitive to how the two threads happen to be scheduled. Messages are
+/// therefore grouped in small batches before being sent, which keeps the output
+/// order untouched while removing most of the cross-thread wake-ups.
+struct StatPrinterTx {
+ tx: mpsc::Sender<Vec<UResult<StatPrintInfo>>>,
+ buffer: RefCell<Vec<UResult<StatPrintInfo>>>,
+}
+
+impl StatPrinterTx {
+ fn new(tx: mpsc::Sender<Vec<UResult<StatPrintInfo>>>) -> Self {
+ Self {
+ tx,
+ buffer: RefCell::new(Vec::with_capacity(PRINT_BATCH_SIZE)),
+ }
+ }
+
+ fn send(&self, message: UResult<StatPrintInfo>) -> Result<(), Box<SendError>> {
+ let mut buffer = self.buffer.borrow_mut();
+ buffer.push(message);
+ if buffer.len() < PRINT_BATCH_SIZE {
+ return Ok(());
+ }
+ let batch = std::mem::replace(&mut *buffer, Vec::with_capacity(PRINT_BATCH_SIZE));
+ Self::send_batch(&self.tx, batch)
+ }
+
+ /// Hand over the messages buffered so far, if any.
+ fn flush(&self) -> Result<(), Box<SendError>> {
+ let batch = std::mem::take(&mut *self.buffer.borrow_mut());
+ if batch.is_empty() {
+ return Ok(());
+ }
+ Self::send_batch(&self.tx, batch)
+ }
+
+ fn send_batch(
+ tx: &mpsc::Sender<Vec<UResult<StatPrintInfo>>>,
+ batch: Vec<UResult<StatPrintInfo>>,
+ ) -> Result<(), Box<SendError>> {
+ // The printing thread is gone: report the failure with the first message
+ // that could not be delivered, like an unbuffered send would.
+ tx.send(batch).map_err(|mpsc::SendError(batch)| {
+ let message = batch
+ .into_iter()
+ .next()
+ .unwrap_or_else(|| Err(USimpleError::new(1, String::new())));
+ Box::new(mpsc::SendError(message))
+ })
+ }
+}
+
+impl Drop for StatPrinterTx {
+ fn drop(&mut self) {
+ // Deliver whatever is left; a failure here means the printing thread is
+ // already gone, which is reported when it is joined.
+ let _ = self.flush();
+ }
+}
+
impl StatPrinter {
fn choose_size(&self, stat: &Stat) -> u64 {
if self.inodes {
@@ -840,13 +907,11 @@ impl StatPrinter {
}
}
- fn print_stats(&self, rx: &mpsc::Receiver<UResult<StatPrintInfo>>) -> UResult<()> {
+ fn print_stats(&self, rx: &mpsc::Receiver<Vec<UResult<StatPrintInfo>>>) -> UResult<()> {
let mut grand_total = 0;
- loop {
- let received = rx.recv();
-
- match received {
- Ok(message) => match message {
+ for batch in rx {
+ for message in batch {
+ match message {
Ok(stat_info) => {
let size = self.choose_size(&stat_info.stat);
@@ -866,8 +931,7 @@ impl StatPrinter {
}
}
Err(e) => show!(e),
- },
- Err(_) => break,
+ }
}
}
@@ -1141,7 +1205,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
}
// Use separate thread to print output, so we can print finished results while computation is still running
- let (print_tx, rx) = mpsc::channel::<UResult<StatPrintInfo>>();
+ let (tx, rx) = mpsc::channel::<Vec<UResult<StatPrintInfo>>>();
+ let print_tx = StatPrinterTx::new(tx);
let printing_thread = thread::spawn(move || stat_printer.print_stats(&rx));
// Check existence of path provided in argument
@@ -1243,6 +1308,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
}
}
+ print_tx
+ .flush()
+ .map_err(|e| USimpleError::new(1, e.to_string()))?;
drop(print_tx);
printing_threadSummary
- Warming up the performance wizard...
- Analyzing the du benchmarks — Reviewed du_bench.rs and the fs_tree helpers
- Measuring baseline stability — Found that only the balanced-tree du benchmarks vary run-to-run; the wide/deep-tree ones are reproducible to ~0.1%
- Isolating the source of the variance — The noisy benchmarks reuse one shared tree across iterations; the stable ones build a fresh input per iteration
- Validating the reworked benchmarks — Traced the instability to futex wake-ups on du's print channel: it is the only syscall whose count changes between identical runs (5366 vs 6154)
- Fixing the root cause in du — Batched the print-channel messages, validated output against the previous binary and the du test suite
-
Opening the follow-up pull request — No push access to uutils/coreutils from this environment, so the change is delivered as a patch in the PR comment instead - Handling comment — Comment handling finished successfully
Learnings updated. See what CodSpeed remembers about this repository.
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.
b9f7518 to
79d083b
Compare
1 participant
No description provided.