Skip to content

Commit

Permalink
ignore: fix reference cycle for compiled matchers
Browse files Browse the repository at this point in the history
It looks like there is a reference cycle caused by the compiled
matchers (compiled HashMap holds ref to Ignore and Ignore holds ref
to HashMap). Using weak refs fixes issue BurntSushi#2690 in my test project.
Also confirmed via before and after when profiling the code, see the
attached screenshots in BurntSushi#2692.

Fixes BurntSushi#2690
  • Loading branch information
fe9lix authored and BurntSushi committed Jan 6, 2024
1 parent 67dd809 commit 338a9e5
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 7 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@
============
This is a minor release with a few small new features and bug fixes.

Bug fixes:

* [BUG #2664](https://github.com/BurntSushi/ripgrep/issues/2690):
Fix unbounded memory growth in the `ignore` crate.

Feature enhancements:

* [FEATURE #2684](https://github.com/BurntSushi/ripgrep/issues/2684):
Expand Down
20 changes: 13 additions & 7 deletions crates/ignore/src/dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use std::{
fs::{File, FileType},
io::{self, BufRead},
path::{Path, PathBuf},
sync::{Arc, RwLock},
sync::{Arc, RwLock, Weak},
};

use crate::{
Expand Down Expand Up @@ -101,7 +101,7 @@ struct IgnoreInner {
/// Note that this is never used during matching, only when adding new
/// parent directory matchers. This avoids needing to rebuild glob sets for
/// parent directories if many paths are being searched.
compiled: Arc<RwLock<HashMap<OsString, Ignore>>>,
compiled: Arc<RwLock<HashMap<OsString, Weak<IgnoreInner>>>>,
/// The path to the directory that this matcher was built from.
dir: PathBuf,
/// An override matcher (default is empty).
Expand Down Expand Up @@ -200,9 +200,11 @@ impl Ignore {
let mut ig = self.clone();
for parent in parents.into_iter().rev() {
let mut compiled = self.0.compiled.write().unwrap();
if let Some(prebuilt) = compiled.get(parent.as_os_str()) {
ig = prebuilt.clone();
continue;
if let Some(weak) = compiled.get(parent.as_os_str()) {
if let Some(prebuilt) = weak.upgrade() {
ig = Ignore(prebuilt);
continue;
}
}
let (mut igtmp, err) = ig.add_child_path(parent);
errs.maybe_push(err);
Expand All @@ -214,8 +216,12 @@ impl Ignore {
} else {
false
};
ig = Ignore(Arc::new(igtmp));
compiled.insert(parent.as_os_str().to_os_string(), ig.clone());
let ig_arc = Arc::new(igtmp);
ig = Ignore(ig_arc.clone());
compiled.insert(
parent.as_os_str().to_os_string(),
Arc::downgrade(&ig_arc),
);
}
(ig, errs.into_error_option())
}
Expand Down

0 comments on commit 338a9e5

Please sign in to comment.