Skip to content

Commit 8c6cd69

Browse files
rm: replace recursive traversal with iterative stack to reduce allocations
Addresses #11222. The previous safe_remove_dir_recursive_impl used recursive call frames and allocated a fresh PathBuf per directory level, an eager Vec<OsString> per level, and a PathBuf::join() per entry. This commit rewrites it as an explicit iterative traversal to reduce heap pressure: - One shared PathBuf for the entire traversal (push/pop per entry) instead of one join() allocation per child. - Vec<StackFrame> work-stack with capacity(32) pre-allocation instead of recursive call frames, eliminating per-level frame overhead and preventing stack overflow on deep trees. - Lazy DirIter (nix OwningIter / getdents) instead of eagerly collecting all children into a Vec<OsString> per level. - Single fd per StackFrame: DirFd::into_iter_dir transfers ownership without dup(2). - StackFrame::dir_path is Option<PathBuf>: child frames start with None (no allocation); populated lazily by try_reclaim_fd only if the frame is demoted to Drained. Fd budget: each Live frame holds exactly one open fd. When the process runs out of file descriptors (EMFILE/ENFILE), try_reclaim_fd demotes the oldest Live frame to Drained — its remaining entries are materialised into a Vec<OsString> and its fd is closed. Subsequent fd-requiring operations on a Drained frame re-open the directory from dir_path on demand. This allows traversal of trees of arbitrary depth at the cost of one extra openat(2) per entry in a drained frame. Security: open_child_iter uses SymlinkBehavior::NoFollow (O_NOFOLLOW | O_DIRECTORY) when opening child directories. stat_at(NoFollow) confirms the entry is a real directory; the subsequent open with O_NOFOLLOW ensures a concurrent symlink swap between the stat and the open is rejected by the kernel with ELOOP/ENOTDIR rather than silently followed. Drained frame helpers (frame_stat_at, frame_open_child, frame_unlink_at) re-open the directory with O_NOFOLLOW, protecting the final path component against concurrent symlink swaps during EMFILE recovery. Bug fix: thread progress_bar through safe_remove_dir_recursive_impl and handle_unlink so pb.inc() is called for every file and subdirectory unlinked during traversal. Previously the bar only ticked once (when the root directory itself was removed), leaving it frozen for large trees. Bug fix: readdir errors in try_reclaim_fd are now reported via show_error! and set had_error=true on the frame, preventing silent entry loss during EMFILE recovery. Previously .filter_map(|r| r.ok()) discarded errors, causing files to be silently skipped. Other changes: - Add DirIter to uucore::safe_traversal: lazy iterator over directory entries exposing stat_at, open_child_iter, and unlink_at so all directory operations go through a single fd. - Add FrameIter enum (Live(DirIter) | Drained(vec::IntoIter<OsString>)) and try_reclaim_fd / frame_stat_at / frame_open_child / frame_unlink_at helpers for the EMFILE fallback path. - Add rm_alloc_count bench (counting GlobalAlloc) to measure allocation reduction directly. - Add test_recursive_deep_tree_no_stack_overflow (#[ignore], 800-level deep tree, run manually). - Add test_recursive_emfile_fd_recycling: caps RLIMIT_NOFILE at 30, removes a 40-level tree, exercises the Drained fallback end-to-end. - Add test_recursive_interactive_decline_child_no_error covering the interactive-decline path without propagating a spurious error. - Restore #[test] on test_recursive_symlink_loop accidentally dropped when the preceding test was inserted. - Document fd budget, push/pop invariants, and TOCTOU windows inline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 12485f2 commit 8c6cd69

5 files changed

Lines changed: 949 additions & 87 deletions

File tree

src/uu/rm/Cargo.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,3 +46,7 @@ uucore = { workspace = true, features = ["benchmark"] }
4646
[[bench]]
4747
name = "rm_bench"
4848
harness = false
49+
50+
[[bench]]
51+
name = "rm_alloc_count"
52+
harness = false
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
// This file is part of the uutils coreutils package.
2+
//
3+
// For the full copyright and license information, please view the LICENSE
4+
// file that was distributed with this source code.
5+
//
6+
// Allocation-counting benchmark for `rm -r`.
7+
//
8+
// Run with:
9+
// cargo bench -p uu_rm --bench rm_alloc_count
10+
//
11+
// To compare against the pre-iterative baseline, check out the commit that
12+
// predates "rm: replace recursive traversal with iterative stack + shared
13+
// PathBuf", then run this bench on both sides:
14+
//
15+
// BASE=$(git log --format="%H %s" | \
16+
// grep -m1 "replace recursive traversal" | awk '{print $1}')
17+
// git stash
18+
// git checkout "$BASE^" -- src/uu/rm/src/platform/unix.rs \
19+
// src/uucore/src/lib/features/safe_traversal.rs
20+
// cargo bench -p uu_rm --bench rm_alloc_count
21+
// git checkout HEAD -- src/uu/rm/src/platform/unix.rs \
22+
// src/uucore/src/lib/features/safe_traversal.rs
23+
// git stash pop
24+
// cargo bench -p uu_rm --bench rm_alloc_count
25+
26+
use std::alloc::{GlobalAlloc, Layout, System};
27+
use std::sync::atomic::{AtomicUsize, Ordering};
28+
29+
use tempfile::TempDir;
30+
use uu_rm::uumain;
31+
use uucore::benchmark::{fs_tree, run_util_function};
32+
33+
// ── Counting allocator ────────────────────────────────────────────────────────
34+
35+
static ALLOC_COUNT: AtomicUsize = AtomicUsize::new(0);
36+
37+
struct CountingAlloc;
38+
39+
unsafe impl GlobalAlloc for CountingAlloc {
40+
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
41+
ALLOC_COUNT.fetch_add(1, Ordering::Relaxed);
42+
unsafe { System.alloc(layout) }
43+
}
44+
unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
45+
unsafe { System.dealloc(ptr, layout) }
46+
}
47+
}
48+
49+
#[global_allocator]
50+
static ALLOC: CountingAlloc = CountingAlloc;
51+
52+
// ── Helpers ───────────────────────────────────────────────────────────────────
53+
54+
fn measure(label: &str, f: impl FnOnce()) {
55+
ALLOC_COUNT.store(0, Ordering::Relaxed);
56+
f();
57+
let count = ALLOC_COUNT.load(Ordering::Relaxed);
58+
println!("{label:<50} {count:>8} heap allocations");
59+
}
60+
61+
// ── Scenarios ────────────────────────────────────────────────────────────────
62+
63+
/// Balanced tree: depth=5, branches=5, 10 files/dir → 3 906 dirs, 39 060 files.
64+
/// This is the same structure used by `rm_recursive_tree` in rm_bench.rs.
65+
fn bench_balanced_tree() {
66+
let temp_dir = TempDir::new().unwrap();
67+
let root = temp_dir.path().join("tree");
68+
std::fs::create_dir(&root).unwrap();
69+
fs_tree::create_balanced_tree(&root, 5, 5, 10);
70+
let path = root.to_str().unwrap().to_string();
71+
72+
measure("rm -r balanced(depth=5, branches=5, files=10)", || {
73+
run_util_function(uumain, &["-r", &path]);
74+
});
75+
}
76+
77+
/// Deep linear chain: 800 levels, 1 file per level.
78+
/// Depth is now capped at ~800 — each StackFrame holds exactly one fd
79+
/// (DirFd::into_iter_dir transfers ownership without dup), so the effective
80+
/// limit is RLIMIT_NOFILE (≈ 1 024) rather than RLIMIT_NOFILE / 2.
81+
fn bench_deep_chain() {
82+
let temp_dir = TempDir::new().unwrap();
83+
let root = temp_dir.path().join("deep");
84+
std::fs::create_dir(&root).unwrap();
85+
fs_tree::create_deep_tree(&root, 800, 1);
86+
let path = root.to_str().unwrap().to_string();
87+
88+
measure("rm -r deep chain (depth=800, 1 file/level) ", || {
89+
run_util_function(uumain, &["-r", &path]);
90+
});
91+
}
92+
93+
fn main() {
94+
println!();
95+
println!("=== rm heap-allocation counts ===");
96+
println!();
97+
bench_balanced_tree();
98+
bench_deep_chain();
99+
println!();
100+
}

0 commit comments

Comments
 (0)