D8315: rust-status: only involve ignore mechanism when needed

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

D8315: rust-status: only involve ignore mechanism when needed

martinvonz (Martin von Zweigbergk)
Alphare created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This prevents unnecessary fallbacks to Python, improving performance for
  `hg update` for instance.
 
  On Mozilla-Central a noop update goes from 1.6s down to 700ms.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-core/src/dirstate/status.rs
  rust/hg-core/src/matchers.rs
  rust/hg-cpython/src/dirstate/status.rs

CHANGE DETAILS

diff --git a/rust/hg-cpython/src/dirstate/status.rs b/rust/hg-cpython/src/dirstate/status.rs
--- a/rust/hg-cpython/src/dirstate/status.rs
+++ b/rust/hg-cpython/src/dirstate/status.rs
@@ -127,7 +127,7 @@
                 &dmap,
                 &matcher,
                 &root_dir,
-                &ignore_files,
+                ignore_files,
                 StatusOptions {
                     check_exec,
                     last_normal_time,
@@ -163,7 +163,7 @@
                 &dmap,
                 &matcher,
                 &root_dir,
-                &ignore_files,
+                ignore_files,
                 StatusOptions {
                     check_exec,
                     last_normal_time,
@@ -217,7 +217,7 @@
                 &dmap,
                 &matcher,
                 &root_dir,
-                &ignore_files,
+                ignore_files,
                 StatusOptions {
                     check_exec,
                     last_normal_time,
diff --git a/rust/hg-core/src/matchers.rs b/rust/hg-core/src/matchers.rs
--- a/rust/hg-core/src/matchers.rs
+++ b/rust/hg-core/src/matchers.rs
@@ -24,12 +24,12 @@
     PatternSyntax,
 };
 
-use micro_timer::timed;
+use std::borrow::ToOwned;
 use std::collections::HashSet;
 use std::fmt::{Display, Error, Formatter};
 use std::iter::FromIterator;
 use std::ops::Deref;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 #[derive(Debug, PartialEq)]
 pub enum VisitChildrenSet<'a> {
@@ -478,7 +478,8 @@
         let mut prefixes = vec![];
 
         for SubInclude { prefix, root, path } in subincludes.into_iter() {
-            let (match_fn, warnings) = get_ignore_function(&[path], root)?;
+            let (match_fn, warnings) =
+                get_ignore_function(vec![path.to_path_buf()], root)?;
             all_warnings.extend(warnings);
             prefixes.push(prefix.to_owned());
             submatchers.insert(prefix.to_owned(), match_fn);
@@ -549,12 +550,11 @@
 /// Parses all "ignore" files with their recursive includes and returns a
 /// function that checks whether a given file (in the general sense) should be
 /// ignored.
-#[timed]
 pub fn get_ignore_function<'a>(
-    all_pattern_files: &[impl AsRef<Path>],
+    all_pattern_files: Vec<PathBuf>,
     root_dir: impl AsRef<Path>,
 ) -> PatternResult<(
-    impl for<'r> Fn(&'r HgPath) -> bool + Sync,
+    Box<dyn for<'r> Fn(&'r HgPath) -> bool + Sync + 'a>,
     Vec<PatternFileWarning>,
 )> {
     let mut all_patterns = vec![];
@@ -564,12 +564,15 @@
         let (patterns, warnings) =
             get_patterns_from_file(pattern_file, &root_dir)?;
 
-        all_patterns.extend(patterns);
+        all_patterns.extend(patterns.to_owned());
         all_warnings.extend(warnings);
     }
     let (matcher, warnings) = IncludeMatcher::new(all_patterns, root_dir)?;
     all_warnings.extend(warnings);
-    Ok((move |path: &HgPath| matcher.matches(path), all_warnings))
+    Ok((
+        Box::new(move |path: &HgPath| matcher.matches(path)),
+        all_warnings,
+    ))
 }
 
 impl<'a> IncludeMatcher<'a> {
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
@@ -33,7 +33,7 @@
     fs::{read_dir, DirEntry},
     io::ErrorKind,
     ops::Deref,
-    path::Path,
+    path::{Path, PathBuf},
 };
 
 /// Wrong type of file from a `BadMatch`
@@ -94,6 +94,9 @@
 }
 
 type IoResult<T> = std::io::Result<T>;
+/// `Box<dyn Trait>` is syntactic sugar for `Box<dyn Trait, 'static>`, so add
+/// an explicit lifetime here to not fight `'static` bounds "out of nowhere".
+type IgnoreFnType<'a> = Box<dyn for<'r> Fn(&'r HgPath) -> bool + Sync + 'a>;
 
 /// Dates and times that are outside the 31-bit signed range are compared
 /// modulo 2^31. This should prevent hg from behaving badly with very large
@@ -312,8 +315,8 @@
     root_dir: impl AsRef<Path> + Sync + Send + Copy + 'a,
     dmap: &'a DirstateMap,
     old_results: &'a FastHashMap<Cow<HgPath>, Dispatch>,
-    ignore_fn: &'a (impl for<'r> Fn(&'r HgPath) -> bool + Sync),
-    dir_ignore_fn: &'a (impl for<'r> Fn(&'r HgPath) -> bool + Sync),
+    ignore_fn: &'a IgnoreFnType,
+    dir_ignore_fn: &'a IgnoreFnType,
     options: StatusOptions,
     filename: HgPathBuf,
     dir_entry: DirEntry,
@@ -393,8 +396,8 @@
     root_dir: impl AsRef<Path> + Sync + Send + Copy + 'a,
     dmap: &'a DirstateMap,
     old_results: &'a FastHashMap<Cow<HgPath>, Dispatch>,
-    ignore_fn: &'a (impl for<'r> Fn(&'r HgPath) -> bool + Sync),
-    dir_ignore_fn: &'a (impl for<'r> Fn(&'r HgPath) -> bool + Sync),
+    ignore_fn: &'a IgnoreFnType,
+    dir_ignore_fn: &'a IgnoreFnType,
     options: StatusOptions,
     entry_option: Option<&'a DirstateEntry>,
     directory: HgPathBuf,
@@ -439,8 +442,8 @@
     dmap: &'a DirstateMap,
     directory: impl AsRef<HgPath>,
     old_results: &FastHashMap<Cow<'a, HgPath>, Dispatch>,
-    ignore_fn: &(impl for<'r> Fn(&'r HgPath) -> bool + Sync),
-    dir_ignore_fn: &(impl for<'r> Fn(&'r HgPath) -> bool + Sync),
+    ignore_fn: &IgnoreFnType,
+    dir_ignore_fn: &IgnoreFnType,
     options: StatusOptions,
 ) -> IoResult<()> {
     let directory = directory.as_ref();
@@ -522,8 +525,8 @@
     dmap: &'a DirstateMap,
     path: impl AsRef<HgPath>,
     old_results: &FastHashMap<Cow<'a, HgPath>, Dispatch>,
-    ignore_fn: &(impl for<'r> Fn(&'r HgPath) -> bool + Sync),
-    dir_ignore_fn: &(impl for<'r> Fn(&'r HgPath) -> bool + Sync),
+    ignore_fn: &IgnoreFnType,
+    dir_ignore_fn: &IgnoreFnType,
     options: StatusOptions,
     results: &mut Vec<(Cow<'a, HgPath>, Dispatch)>,
 ) -> IoResult<()> {
@@ -805,26 +808,39 @@
     dmap: &'a DirstateMap,
     matcher: &'b (impl Matcher + Sync),
     root_dir: impl AsRef<Path> + Sync + Send + Copy + 'c,
-    ignore_files: &[impl AsRef<Path> + 'c],
+    ignore_files: Vec<PathBuf>,
     options: StatusOptions,
 ) -> StatusResult<(
     (Vec<Cow<'c, HgPath>>, DirstateStatus<'c>),
     Vec<PatternFileWarning>,
 )> {
-    let (ignore_fn, warnings) = get_ignore_function(&ignore_files, root_dir)?;
+    // Needs to outlive `dir_ignore_fn` since it's captured.
+    let mut ignore_fn: IgnoreFnType;
+
+    // Only involve real ignore mechanism if we're listing unknowns or ignored.
+    let (dir_ignore_fn, warnings): (IgnoreFnType, _) = if options.list_ignored
+        || options.list_unknown
+    {
+        let (ignore, warnings) = get_ignore_function(ignore_files, root_dir)?;
 
-    // Is the path or one of its ancestors ignored?
-    let dir_ignore_fn = |dir: &_| {
-        if ignore_fn(dir) {
-            true
-        } else {
-            for p in find_dirs(dir) {
-                if ignore_fn(p) {
-                    return true;
+        ignore_fn = ignore;
+        let dir_ignore_fn = Box::new(|dir: &_| {
+            // Is the path or one of its ancestors ignored?
+            if ignore_fn(dir) {
+                true
+            } else {
+                for p in find_dirs(dir) {
+                    if ignore_fn(p) {
+                        return true;
+                    }
                 }
+                false
             }
-            false
-        }
+        });
+        (dir_ignore_fn, warnings)
+    } else {
+        ignore_fn = Box::new(|&_| true);
+        (Box::new(|&_| true), vec![])
     };
 
     let files = matcher.file_set();



To: Alphare, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D8315: rust-status: only involve ignore mechanism when needed

martinvonz (Martin von Zweigbergk)
Closed by commit rHGe62052d0f377: rust-status: only involve ignore mechanism when needed (authored by Alphare).
This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state "Needs Review".

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D8315?vs=20855&id=20879

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8315/new/

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

AFFECTED FILES
  rust/hg-core/src/dirstate/status.rs
  rust/hg-core/src/matchers.rs
  rust/hg-cpython/src/dirstate/status.rs

CHANGE DETAILS

diff --git a/rust/hg-cpython/src/dirstate/status.rs b/rust/hg-cpython/src/dirstate/status.rs
--- a/rust/hg-cpython/src/dirstate/status.rs
+++ b/rust/hg-cpython/src/dirstate/status.rs
@@ -127,7 +127,7 @@
                 &dmap,
                 &matcher,
                 &root_dir,
-                &ignore_files,
+                ignore_files,
                 StatusOptions {
                     check_exec,
                     last_normal_time,
@@ -163,7 +163,7 @@
                 &dmap,
                 &matcher,
                 &root_dir,
-                &ignore_files,
+                ignore_files,
                 StatusOptions {
                     check_exec,
                     last_normal_time,
@@ -217,7 +217,7 @@
                 &dmap,
                 &matcher,
                 &root_dir,
-                &ignore_files,
+                ignore_files,
                 StatusOptions {
                     check_exec,
                     last_normal_time,
diff --git a/rust/hg-core/src/matchers.rs b/rust/hg-core/src/matchers.rs
--- a/rust/hg-core/src/matchers.rs
+++ b/rust/hg-core/src/matchers.rs
@@ -24,12 +24,12 @@
     PatternSyntax,
 };
 
-use micro_timer::timed;
+use std::borrow::ToOwned;
 use std::collections::HashSet;
 use std::fmt::{Display, Error, Formatter};
 use std::iter::FromIterator;
 use std::ops::Deref;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 #[derive(Debug, PartialEq)]
 pub enum VisitChildrenSet<'a> {
@@ -507,7 +507,8 @@
         let mut prefixes = vec![];
 
         for SubInclude { prefix, root, path } in subincludes.into_iter() {
-            let (match_fn, warnings) = get_ignore_function(&[path], root)?;
+            let (match_fn, warnings) =
+                get_ignore_function(vec![path.to_path_buf()], root)?;
             all_warnings.extend(warnings);
             prefixes.push(prefix.to_owned());
             submatchers.insert(prefix.to_owned(), match_fn);
@@ -578,12 +579,11 @@
 /// Parses all "ignore" files with their recursive includes and returns a
 /// function that checks whether a given file (in the general sense) should be
 /// ignored.
-#[timed]
 pub fn get_ignore_function<'a>(
-    all_pattern_files: &[impl AsRef<Path>],
+    all_pattern_files: Vec<PathBuf>,
     root_dir: impl AsRef<Path>,
 ) -> PatternResult<(
-    impl for<'r> Fn(&'r HgPath) -> bool + Sync,
+    Box<dyn for<'r> Fn(&'r HgPath) -> bool + Sync + 'a>,
     Vec<PatternFileWarning>,
 )> {
     let mut all_patterns = vec![];
@@ -593,12 +593,15 @@
         let (patterns, warnings) =
             get_patterns_from_file(pattern_file, &root_dir)?;
 
-        all_patterns.extend(patterns);
+        all_patterns.extend(patterns.to_owned());
         all_warnings.extend(warnings);
     }
     let (matcher, warnings) = IncludeMatcher::new(all_patterns, root_dir)?;
     all_warnings.extend(warnings);
-    Ok((move |path: &HgPath| matcher.matches(path), all_warnings))
+    Ok((
+        Box::new(move |path: &HgPath| matcher.matches(path)),
+        all_warnings,
+    ))
 }
 
 impl<'a> IncludeMatcher<'a> {
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
@@ -33,7 +33,7 @@
     fs::{read_dir, DirEntry},
     io::ErrorKind,
     ops::Deref,
-    path::Path,
+    path::{Path, PathBuf},
 };
 
 /// Wrong type of file from a `BadMatch`
@@ -94,6 +94,9 @@
 }
 
 type IoResult<T> = std::io::Result<T>;
+/// `Box<dyn Trait>` is syntactic sugar for `Box<dyn Trait, 'static>`, so add
+/// an explicit lifetime here to not fight `'static` bounds "out of nowhere".
+type IgnoreFnType<'a> = Box<dyn for<'r> Fn(&'r HgPath) -> bool + Sync + 'a>;
 
 /// Dates and times that are outside the 31-bit signed range are compared
 /// modulo 2^31. This should prevent hg from behaving badly with very large
@@ -312,8 +315,8 @@
     root_dir: impl AsRef<Path> + Sync + Send + Copy + 'a,
     dmap: &'a DirstateMap,
     old_results: &'a FastHashMap<Cow<HgPath>, Dispatch>,
-    ignore_fn: &'a (impl for<'r> Fn(&'r HgPath) -> bool + Sync),
-    dir_ignore_fn: &'a (impl for<'r> Fn(&'r HgPath) -> bool + Sync),
+    ignore_fn: &'a IgnoreFnType,
+    dir_ignore_fn: &'a IgnoreFnType,
     options: StatusOptions,
     filename: HgPathBuf,
     dir_entry: DirEntry,
@@ -393,8 +396,8 @@
     root_dir: impl AsRef<Path> + Sync + Send + Copy + 'a,
     dmap: &'a DirstateMap,
     old_results: &'a FastHashMap<Cow<HgPath>, Dispatch>,
-    ignore_fn: &'a (impl for<'r> Fn(&'r HgPath) -> bool + Sync),
-    dir_ignore_fn: &'a (impl for<'r> Fn(&'r HgPath) -> bool + Sync),
+    ignore_fn: &'a IgnoreFnType,
+    dir_ignore_fn: &'a IgnoreFnType,
     options: StatusOptions,
     entry_option: Option<&'a DirstateEntry>,
     directory: HgPathBuf,
@@ -439,8 +442,8 @@
     dmap: &'a DirstateMap,
     directory: impl AsRef<HgPath>,
     old_results: &FastHashMap<Cow<'a, HgPath>, Dispatch>,
-    ignore_fn: &(impl for<'r> Fn(&'r HgPath) -> bool + Sync),
-    dir_ignore_fn: &(impl for<'r> Fn(&'r HgPath) -> bool + Sync),
+    ignore_fn: &IgnoreFnType,
+    dir_ignore_fn: &IgnoreFnType,
     options: StatusOptions,
 ) -> IoResult<()> {
     let directory = directory.as_ref();
@@ -522,8 +525,8 @@
     dmap: &'a DirstateMap,
     path: impl AsRef<HgPath>,
     old_results: &FastHashMap<Cow<'a, HgPath>, Dispatch>,
-    ignore_fn: &(impl for<'r> Fn(&'r HgPath) -> bool + Sync),
-    dir_ignore_fn: &(impl for<'r> Fn(&'r HgPath) -> bool + Sync),
+    ignore_fn: &IgnoreFnType,
+    dir_ignore_fn: &IgnoreFnType,
     options: StatusOptions,
     results: &mut Vec<(Cow<'a, HgPath>, Dispatch)>,
 ) -> IoResult<()> {
@@ -805,26 +808,39 @@
     dmap: &'a DirstateMap,
     matcher: &'b (impl Matcher + Sync),
     root_dir: impl AsRef<Path> + Sync + Send + Copy + 'c,
-    ignore_files: &[impl AsRef<Path> + 'c],
+    ignore_files: Vec<PathBuf>,
     options: StatusOptions,
 ) -> StatusResult<(
     (Vec<Cow<'c, HgPath>>, DirstateStatus<'c>),
     Vec<PatternFileWarning>,
 )> {
-    let (ignore_fn, warnings) = get_ignore_function(&ignore_files, root_dir)?;
+    // Needs to outlive `dir_ignore_fn` since it's captured.
+    let mut ignore_fn: IgnoreFnType;
+
+    // Only involve real ignore mechanism if we're listing unknowns or ignored.
+    let (dir_ignore_fn, warnings): (IgnoreFnType, _) = if options.list_ignored
+        || options.list_unknown
+    {
+        let (ignore, warnings) = get_ignore_function(ignore_files, root_dir)?;
 
-    // Is the path or one of its ancestors ignored?
-    let dir_ignore_fn = |dir: &_| {
-        if ignore_fn(dir) {
-            true
-        } else {
-            for p in find_dirs(dir) {
-                if ignore_fn(p) {
-                    return true;
+        ignore_fn = ignore;
+        let dir_ignore_fn = Box::new(|dir: &_| {
+            // Is the path or one of its ancestors ignored?
+            if ignore_fn(dir) {
+                true
+            } else {
+                for p in find_dirs(dir) {
+                    if ignore_fn(p) {
+                        return true;
+                    }
                 }
+                false
             }
-            false
-        }
+        });
+        (dir_ignore_fn, warnings)
+    } else {
+        ignore_fn = Box::new(|&_| true);
+        (Box::new(|&_| true), vec![])
     };
 
     let files = matcher.file_set();



To: Alphare, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel