Patchwork D8213: rust-pathauditor: use interior mutability for use in multi-threaded contexts

login
register
mail settings
Submitter phabricator
Date March 4, 2020, 2:46 p.m.
Message ID <differential-rev-PHID-DREV-q764pmli3higdnek5c65-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45453/
State Superseded
Headers show

Comments

phabricator - March 4, 2020, 2:46 p.m.
Alphare created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The usual recommendation for using `RwLock` or `Mutex` is that if there are
  about as many write as there are reads, use `Mutex`, and if there are more
  reads than writes, use `RwLock`.
  
  If after the main bottleneck (i.e. parallel traversal) is removed this shows
  up on profiles, we should investigate using the `parking_lot` since we don't
  need a poisoning API, or maybe move to different types of caches entirely.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8213

AFFECTED FILES
  rust/hg-core/src/dirstate/status.rs
  rust/hg-core/src/utils/files.rs
  rust/hg-core/src/utils/path_auditor.rs

CHANGE DETAILS




To: Alphare, #hg-reviewers
Cc: mercurial-devel

Patch

diff --git a/rust/hg-core/src/utils/path_auditor.rs b/rust/hg-core/src/utils/path_auditor.rs
--- a/rust/hg-core/src/utils/path_auditor.rs
+++ b/rust/hg-core/src/utils/path_auditor.rs
@@ -11,15 +11,17 @@ 
     find_slice_in_slice,
     hg_path::{hg_path_to_path_buf, HgPath, HgPathBuf, HgPathError},
 };
+use std::borrow::ToOwned;
 use std::collections::HashSet;
 use std::path::{Path, PathBuf};
+use std::sync::{Mutex, RwLock};
 
 /// Ensures that a path is valid for use in the repository i.e. does not use
 /// any banned components, does not traverse a symlink, etc.
 #[derive(Debug, Default)]
 pub struct PathAuditor {
-    audited: HashSet<HgPathBuf>,
-    audited_dirs: HashSet<HgPathBuf>,
+    audited: Mutex<HashSet<HgPathBuf>>,
+    audited_dirs: RwLock<HashSet<HgPathBuf>>,
     root: PathBuf,
 }
 
@@ -31,7 +33,7 @@ 
         }
     }
     pub fn audit_path(
-        &mut self,
+        &self,
         path: impl AsRef<HgPath>,
     ) -> Result<(), HgPathError> {
         // TODO windows "localpath" normalization
@@ -40,7 +42,7 @@ 
             return Ok(());
         }
         // TODO case normalization
-        if self.audited.contains(path) {
+        if self.audited.lock().unwrap().contains(path) {
             return Ok(());
         }
         // AIX ignores "/" at end of path, others raise EISDIR.
@@ -113,14 +115,14 @@ 
         for index in 0..parts.len() {
             let prefix = &parts[..index + 1].join(&b'/');
             let prefix = HgPath::new(prefix);
-            if self.audited_dirs.contains(prefix) {
+            if self.audited_dirs.read().unwrap().contains(prefix) {
                 continue;
             }
             self.check_filesystem(&prefix, &path)?;
-            self.audited_dirs.insert(prefix.to_owned());
+            self.audited_dirs.write().unwrap().insert(prefix.to_owned());
         }
 
-        self.audited.insert(path.to_owned());
+        self.audited.lock().unwrap().insert(path.to_owned());
 
         Ok(())
     }
@@ -171,7 +173,7 @@ 
         Ok(())
     }
 
-    pub fn check(&mut self, path: impl AsRef<HgPath>) -> bool {
+    pub fn check(&self, path: impl AsRef<HgPath>) -> bool {
         self.audit_path(path).is_ok()
     }
 }
@@ -184,7 +186,7 @@ 
 
     #[test]
     fn test_path_auditor() {
-        let mut auditor = PathAuditor::new(get_path_from_bytes(b"/tmp"));
+        let auditor = PathAuditor::new(get_path_from_bytes(b"/tmp"));
 
         let path = HgPath::new(b".hg/00changelog.i");
         assert_eq!(
diff --git a/rust/hg-core/src/utils/files.rs b/rust/hg-core/src/utils/files.rs
--- a/rust/hg-core/src/utils/files.rs
+++ b/rust/hg-core/src/utils/files.rs
@@ -210,7 +210,7 @@ 
     } else {
         name.to_owned()
     };
-    let mut auditor = PathAuditor::new(&root);
+    let auditor = PathAuditor::new(&root);
     if name != root && name.starts_with(&root) {
         let name = name.strip_prefix(&root).unwrap();
         auditor.audit_path(path_to_hg_path_buf(name)?)?;
diff --git a/rust/hg-core/src/dirstate/status.rs b/rust/hg-core/src/dirstate/status.rs
--- a/rust/hg-core/src/dirstate/status.rs
+++ b/rust/hg-core/src/dirstate/status.rs
@@ -680,7 +680,7 @@ 
             // We walked all dirs under the roots that weren't ignored, and
             // everything that matched was stat'ed and is already in results.
             // The rest must thus be ignored or under a symlink.
-            let mut path_auditor = PathAuditor::new(root_dir);
+            let path_auditor = PathAuditor::new(root_dir);
 
             for (ref filename, entry) in to_visit {
                 // Report ignored items in the dmap as long as they are not