diff --git a/CHANGELOG.md b/CHANGELOG.md index 63c04ea60..9d47a8caa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ Unreleased changes. Release notes have not yet been written. `rg -B1 -A2`. That is, `-A` and `-B` no longer completely override `-C`. Instead, they only partially override `-C`. +Performance improvements: + +* [PERF #2591](https://github.com/BurntSushi/ripgrep/pull/2591): + Parallel directory traversal now uses work stealing for faster searches. + Feature enhancements: * Added or improved file type filtering for Ada, DITA, Elixir, Fuchsia, Gentoo, @@ -21,6 +26,8 @@ Feature enhancements: Bug fixes: +* [BUG #1757](https://github.com/BurntSushi/ripgrep/issues/1757): + Fix bug when searching a sub-directory didn't have ignores applied correctly. * [BUG #1891](https://github.com/BurntSushi/ripgrep/issues/1891): Fix bug when using `-w` with a regex that can match the empty string. * [BUG #1911](https://github.com/BurntSushi/ripgrep/issues/1911): diff --git a/Cargo.lock b/Cargo.lock index 259a7c21b..d6fd4508c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -78,6 +78,30 @@ dependencies = [ "crossbeam-utils", ] +[[package]] +name = "crossbeam-deque" +version = "0.8.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ce6fd6f855243022dcecf8702fef0c297d4338e226845fe067f6341ad9fa0cef" +dependencies = [ + "cfg-if", + "crossbeam-epoch", + "crossbeam-utils", +] + +[[package]] +name = "crossbeam-epoch" +version = "0.9.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae211234986c545741a7dc064309f67ee1e5ad243d0e48335adc0484d960bcc7" +dependencies = [ + "autocfg", + "cfg-if", + "crossbeam-utils", + "memoffset", + "scopeguard", +] + [[package]] name = "crossbeam-utils" version = "0.8.16" @@ -224,6 +248,7 @@ name = "ignore" version = "0.4.20" dependencies = [ "crossbeam-channel", + "crossbeam-deque", "globset", "lazy_static", "log", @@ -278,9 +303,9 @@ checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" [[package]] name = "libc" -version = "0.2.147" +version = "0.2.148" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b4668fb0ea861c1df094127ac5f1da3409a82116a4ba74fca2e58ef927159bb3" +checksum = "9cdc71e17332e86d2e1d38c1f99edcb6288ee11b815fb1a4b049eaa2114d369b" [[package]] name = "libm" @@ -309,6 +334,15 @@ dependencies = [ "libc", ] +[[package]] +name = "memoffset" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a634b1c61a95585bd15607c6ab0c4e5b226e695ff2800ba0cdccddf208c406c" +dependencies = [ + "autocfg", +] + [[package]] name = "num-traits" version = "0.2.16" @@ -366,9 +400,9 @@ checksum = "26072860ba924cbfa98ea39c8c19b4dd6a4a25423dbdf219c1eca91aa0cf6964" [[package]] name = "proc-macro2" -version = "1.0.66" +version = "1.0.67" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "18fb31db3f9bddb2ea821cde30a9f70117e3f119938b5ee630b7403aa6e2ead9" +checksum = "3d433d9f1a3e8c1263d9456598b16fec66f4acc9a74dacffd35c7bb09b3a1328" dependencies = [ "unicode-ident", ] @@ -444,6 +478,12 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "scopeguard" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" + [[package]] name = "serde" version = "1.0.188" @@ -466,9 +506,9 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.105" +version = "1.0.107" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "693151e1ac27563d6dbcec9dee9fbd5da8539b20fa14ad3752b2e6d363ace360" +checksum = "6b420ce6e3d8bd882e9b243c6eed35dbc9a6110c9769e74b584e0d68d1f20c65" dependencies = [ "itoa", "ryu", @@ -483,9 +523,9 @@ checksum = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a" [[package]] name = "syn" -version = "2.0.29" +version = "2.0.37" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c324c494eba9d92503e6f1ef2e6df781e78f6a7705a0202d9801b198807d518a" +checksum = "7303ef2c05cd654186cb250d29049a24840ca25d2747c25c0381c8d9e2f582e8" dependencies = [ "proc-macro2", "quote", @@ -494,9 +534,9 @@ dependencies = [ [[package]] name = "termcolor" -version = "1.2.0" +version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "be55cf8942feac5c765c2c993422806843c9a9a45d4d5c407ad6dd2ea95eb9b6" +checksum = "6093bad37da69aab9d123a8091e4be0aa4a03e4d601ec641c327398315f62b64" dependencies = [ "winapi-util", ] @@ -522,9 +562,9 @@ dependencies = [ [[package]] name = "unicode-ident" -version = "1.0.11" +version = "1.0.12" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "301abaae475aa91687eb82514b328ab47a211a533026cb25fc3e519b86adfc3c" +checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b" [[package]] name = "unicode-width" @@ -534,9 +574,9 @@ checksum = "c0edd1e5b14653f783770bce4a4dabb4a5108a5370a5f5d8cfe8710c361f6c8b" [[package]] name = "walkdir" -version = "2.3.3" +version = "2.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "36df944cda56c7d8d8b7496af378e6b16de9284591917d307c9b4d313c44e698" +checksum = "d71d857dc86794ca4c280d616f7da00d2dbfd8cd788846559a6813e6aa4b54ee" dependencies = [ "same-file", "winapi-util", diff --git a/crates/ignore/Cargo.toml b/crates/ignore/Cargo.toml index a9495aa33..c659bb2d4 100644 --- a/crates/ignore/Cargo.toml +++ b/crates/ignore/Cargo.toml @@ -19,6 +19,7 @@ name = "ignore" bench = false [dependencies] +crossbeam-deque = "0.8.3" globset = { version = "0.4.10", path = "../globset" } lazy_static = "1.1" log = "0.4.5" diff --git a/crates/ignore/src/dir.rs b/crates/ignore/src/dir.rs index 2577665d5..c813eb0d5 100644 --- a/crates/ignore/src/dir.rs +++ b/crates/ignore/src/dir.rs @@ -442,7 +442,29 @@ impl Ignore { } if self.0.opts.parents { if let Some(abs_parent_path) = self.absolute_base() { - let path = abs_parent_path.join(path); + // What we want to do here is take the absolute base path of + // this directory and join it with the path we're searching. + // The main issue we want to avoid is accidentally duplicating + // directory components, so we try to strip any common prefix + // off of `path`. Overall, this seems a little ham-fisted, but + // it does fix a nasty bug. It should do fine until we overhaul + // this crate. + let dirpath = self.0.dir.as_path(); + let path_prefix = match strip_prefix("./", dirpath) { + None => dirpath, + Some(stripped_dot_slash) => stripped_dot_slash, + }; + let path = match strip_prefix(path_prefix, path) { + None => abs_parent_path.join(path), + Some(p) => { + let p = match strip_prefix("/", p) { + None => p, + Some(p) => p, + }; + abs_parent_path.join(p) + } + }; + for ig in self.parents().skip_while(|ig| !ig.0.is_absolute_parent) { diff --git a/crates/ignore/src/walk.rs b/crates/ignore/src/walk.rs index 734b87667..fdeea93c4 100644 --- a/crates/ignore/src/walk.rs +++ b/crates/ignore/src/walk.rs @@ -3,14 +3,15 @@ use std::ffi::OsStr; use std::fmt; use std::fs::{self, FileType, Metadata}; use std::io; -use std::iter::FusedIterator; +use std::iter::{self, FusedIterator}; use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use std::thread; use std::time::Duration; use std::vec; +use crossbeam_deque::{Stealer, Worker as Deque}; use same_file::Handle; use walkdir::{self, WalkDir}; @@ -1231,9 +1232,8 @@ impl WalkParallel { /// can be merged together into a single data structure. pub fn visit(mut self, builder: &mut dyn ParallelVisitorBuilder<'_>) { let threads = self.threads(); - let stack = Arc::new(Mutex::new(vec![])); + let mut stack = vec![]; { - let mut stack = stack.lock().unwrap(); let mut visitor = builder.build(); let mut paths = Vec::new().into_iter(); std::mem::swap(&mut paths, &mut self.paths); @@ -1283,14 +1283,14 @@ impl WalkParallel { } // Create the workers and then wait for them to finish. let quit_now = Arc::new(AtomicBool::new(false)); - let num_pending = - Arc::new(AtomicUsize::new(stack.lock().unwrap().len())); + let num_pending = Arc::new(AtomicUsize::new(stack.len())); + let stacks = Stack::new_for_each_thread(threads, stack); std::thread::scope(|s| { - let mut handles = vec![]; - for _ in 0..threads { - let worker = Worker { + let handles: Vec<_> = stacks + .into_iter() + .map(|stack| Worker { visitor: builder.build(), - stack: stack.clone(), + stack, quit_now: quit_now.clone(), num_pending: num_pending.clone(), max_depth: self.max_depth, @@ -1298,9 +1298,9 @@ impl WalkParallel { follow_links: self.follow_links, skip: self.skip.clone(), filter: self.filter.clone(), - }; - handles.push(s.spawn(|| worker.run())); - } + }) + .map(|worker| s.spawn(|| worker.run())) + .collect(); for handle in handles { handle.join().unwrap(); } @@ -1390,6 +1390,73 @@ impl Work { } } +/// A work-stealing stack. +#[derive(Debug)] +struct Stack { + /// This thread's index. + index: usize, + /// The thread-local stack. + deque: Deque, + /// The work stealers. + stealers: Arc<[Stealer]>, +} + +impl Stack { + /// Create a work-stealing stack for each thread. The given messages + /// correspond to the initial paths to start the search at. They will + /// be distributed automatically to each stack in a round-robin fashion. + fn new_for_each_thread(threads: usize, init: Vec) -> Vec { + // Using new_lifo() ensures each worker operates depth-first, not + // breadth-first. We do depth-first because a breadth first traversal + // on wide directories with a lot of gitignores is disastrous (for + // example, searching a directory tree containing all of crates.io). + let deques: Vec> = + iter::repeat_with(Deque::new_lifo).take(threads).collect(); + let stealers = Arc::<[Stealer]>::from( + deques.iter().map(Deque::stealer).collect::>(), + ); + let stacks: Vec = deques + .into_iter() + .enumerate() + .map(|(index, deque)| Stack { + index, + deque, + stealers: stealers.clone(), + }) + .collect(); + // Distribute the initial messages. + init.into_iter() + .zip(stacks.iter().cycle()) + .for_each(|(m, s)| s.push(m)); + stacks + } + + /// Push a message. + fn push(&self, msg: Message) { + self.deque.push(msg); + } + + /// Pop a message. + fn pop(&self) -> Option { + self.deque.pop().or_else(|| self.steal()) + } + + /// Steal a message from another queue. + fn steal(&self) -> Option { + // For fairness, try to steal from index - 1, then index - 2, ... 0, + // then wrap around to len - 1, len - 2, ... index + 1. + let (left, right) = self.stealers.split_at(self.index); + // Don't steal from ourselves + let right = &right[1..]; + + left.iter() + .rev() + .chain(right.iter().rev()) + .map(|s| s.steal_batch_and_pop(&self.deque)) + .find_map(|s| s.success()) + } +} + /// A worker is responsible for descending into directories, updating the /// ignore matchers, producing new work and invoking the caller's callback. /// @@ -1397,13 +1464,13 @@ impl Work { struct Worker<'s> { /// The caller's callback. visitor: Box, - /// A stack of work to do. + /// A work-stealing stack of work to do. /// /// We use a stack instead of a channel because a stack lets us visit /// directories in depth first order. This can substantially reduce peak - /// memory usage by keeping both the number of files path and gitignore + /// memory usage by keeping both the number of file paths and gitignore /// matchers in memory lower. - stack: Arc>>, + stack: Stack, /// Whether all workers should terminate at the next opportunity. Note /// that we need this because we don't want other `Work` to be done after /// we quit. We wouldn't need this if have a priority channel. @@ -1668,20 +1735,17 @@ impl<'s> Worker<'s> { /// Send work. fn send(&self, work: Work) { self.num_pending.fetch_add(1, Ordering::SeqCst); - let mut stack = self.stack.lock().unwrap(); - stack.push(Message::Work(work)); + self.stack.push(Message::Work(work)); } /// Send a quit message. fn send_quit(&self) { - let mut stack = self.stack.lock().unwrap(); - stack.push(Message::Quit); + self.stack.push(Message::Quit); } /// Receive work. fn recv(&self) -> Option { - let mut stack = self.stack.lock().unwrap(); - stack.pop() + self.stack.pop() } /// Signal that work has been finished. diff --git a/tests/regression.rs b/tests/regression.rs index 5ef741cf6..91c374497 100644 --- a/tests/regression.rs +++ b/tests/regression.rs @@ -952,6 +952,19 @@ rgtest!(r1739_replacement_lineterm_match, |dir: Dir, mut cmd: TestCommand| { eqnice!("af\n", cmd.stdout()); }); +// See: https://github.com/BurntSushi/ripgrep/issues/1757 +rgtest!(f1757, |dir: Dir, _: TestCommand| { + dir.create_dir("rust/target"); + dir.create(".ignore", "rust/target"); + dir.create("rust/source.rs", "needle"); + dir.create("rust/target/rustdoc-output.html", "needle"); + + let args = &["--files-with-matches", "needle", "rust"]; + eqnice!("rust/source.rs\n", dir.command().args(args).stdout()); + let args = &["--files-with-matches", "needle", "./rust"]; + eqnice!("./rust/source.rs\n", dir.command().args(args).stdout()); +}); + // See: https://github.com/BurntSushi/ripgrep/issues/1765 rgtest!(r1765, |dir: Dir, mut cmd: TestCommand| { dir.create("test", "\n");