This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7914

rust-matchers: implement `visit_children_set` for `FileMatcher`
ClosedPublic

Authored by Alphare on Jan 16 2020, 5:09 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG54d185eb24b5: rust-matchers: implement `visit_children_set` for `FileMatcher`
Summary

As per the removed inline comment, this will become useful in a future patch
in this series as the IncludeMatcher is introduced.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Jan 16 2020, 5:09 PM
Alphare updated this revision to Diff 19388.Jan 16 2020, 5:12 PM
martinvonz added a subscriber: martinvonz.Jan 17 2020, 1:35 AM
martinvonz added inline comments.
rust/hg-core/src/matchers.rs
164

This will often be called repeatedly, so isn't it better to calculate a map of each parent directory to its VisitChildrenSet value upfront (in FilesMatcher::new())?

Alphare added inline comments.Jan 17 2020, 4:17 AM
rust/hg-core/src/matchers.rs
164

Agreed. In much of this series there exist opportunities for caching/making things run in parallel, etc. With the freeze approaching really fast, I prefer to prioritize getting correct - albeit sub-optimal - code in rather than risking missing the deadline.
My benchmarks of the entire series show an improvement in bare hg status in all supported repositories (read: that don't have back-references in their `.hgignore patterns), and no measurable slowdown in others.

martinvonz added inline comments.Jan 17 2020, 11:08 AM
rust/hg-core/src/matchers.rs
164

Yes, code quality is never urgent. The problem is that there isn't much incentive for people to clean it up after. So I'd really prefer to not queue this without the cleanup.

Alphare added inline comments.Jan 17 2020, 11:51 AM
rust/hg-core/src/matchers.rs
164

I don't feel like this is an issue of code quality. This is (probably) an issue of performance, and I have a good incentive for making this code go faster in the future. I don't think this is necessary for now since this is a minor optimization compared to the other ones that are pending.

martinvonz added inline comments.Jan 17 2020, 12:30 PM
rust/hg-core/src/matchers.rs
164

How about we just replace the implementation by VisitChildrenSet::Recursivethen? That's easy to maintain and makes it functionally correct. It's usually fast enough too.

martinvonz added inline comments.Jan 19 2020, 1:29 PM
rust/hg-core/src/matchers.rs
164

Alphare pointed out to me out of band that the Python code already does the same, so I agree that it makes sense to just copy that for now. I'm sorry that I didn't think of checking that before. It would have been even better to fix the Python version first and then copy the good code to Rust, but I'm still fine with doing that later.

PS. I haven't actually reviewed this code. I'll do that next week (and Monday is a holiday for me).

Alphare added a comment.Feb 10 2020, 11:06 AM

@martinvonz Any update on this patch?
Note: I will get to your remarks on the first change of the stack very soon as well.

Alphare added a commit: rHG54d185eb24b5: rust-matchers: implement `visit_children_set` for `FileMatcher`.Feb 11 2020, 12:32 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG54d185eb24b5: rust-matchers: implement `visit_children_set` for `FileMatcher` (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/matchers.rs (150 lines)

Diff 19387

rust/hg-core/src/matchers.rs

Some(&self.files) Some(&self.files)
} }
fn exact_match(&self, filename: impl AsRef<HgPath>) -> bool { fn exact_match(&self, filename: impl AsRef<HgPath>) -> bool {
self.inner_matches(filename) self.inner_matches(filename)
} }
fn matches(&self, filename: impl AsRef<HgPath>) -> bool { fn matches(&self, filename: impl AsRef<HgPath>) -> bool {
self.inner_matches(filename) self.inner_matches(filename)
} }
fn visit_children_set( fn visit_children_set(
martinvonzUnsubmitted
Not Done

This will often be called repeatedly, so isn't it better to calculate a map of each parent directory to its VisitChildrenSet value upfront (in FilesMatcher::new())?

martinvonz: This will often be called repeatedly, so isn't it better to calculate a map of each parent…
AlphareAuthorUnsubmitted
Done

Agreed. In much of this series there exist opportunities for caching/making things run in parallel, etc. With the freeze approaching really fast, I prefer to prioritize getting correct - albeit sub-optimal - code in rather than risking missing the deadline.
My benchmarks of the entire series show an improvement in bare hg status in all supported repositories (read: that don't have back-references in their `.hgignore patterns), and no measurable slowdown in others.

Alphare: Agreed. In much of this series there exist opportunities for caching/making things run in…
martinvonzUnsubmitted
Not Done

Yes, code quality is never urgent. The problem is that there isn't much incentive for people to clean it up after. So I'd really prefer to not queue this without the cleanup.

martinvonz: Yes, code quality is never urgent. The problem is that there isn't much incentive for people to…
AlphareAuthorUnsubmitted
Done

I don't feel like this is an issue of code quality. This is (probably) an issue of performance, and I have a good incentive for making this code go faster in the future. I don't think this is necessary for now since this is a minor optimization compared to the other ones that are pending.

Alphare: I don't feel like this is an issue of code quality. This is (probably) an issue of performance…
martinvonzUnsubmitted
Not Done

How about we just replace the implementation by VisitChildrenSet::Recursivethen? That's easy to maintain and makes it functionally correct. It's usually fast enough too.

martinvonz: How about we just replace the implementation by `VisitChildrenSet::Recursive`then? That's easy…
martinvonzUnsubmitted
Not Done

Alphare pointed out to me out of band that the Python code already does the same, so I agree that it makes sense to just copy that for now. I'm sorry that I didn't think of checking that before. It would have been even better to fix the Python version first and then copy the good code to Rust, but I'm still fine with doing that later.

PS. I haven't actually reviewed this code. I'll do that next week (and Monday is a holiday for me).

martinvonz: Alphare pointed out to me out of band that the Python code already does the same, so I agree…
&self, &self,
_directory: impl AsRef<HgPath>, directory: impl AsRef<HgPath>,
) -> VisitChildrenSet { ) -> VisitChildrenSet {
// TODO implement once we have `status.traverse` if self.files.is_empty() || !self.dirs.contains(&directory) {
// This is useless until unknown files are taken into account return VisitChildrenSet::Empty;
// Which will not need to happen before the `IncludeMatcher`. }
unimplemented!() let dirs_as_set = self.dirs.iter().map(|k| k.deref()).collect();
let mut candidates: HashSet<&HgPath> =
self.files.union(&dirs_as_set).map(|k| *k).collect();
candidates.remove(HgPath::new(b""));
if !directory.as_ref().is_empty() {
let directory = [directory.as_ref().as_bytes(), b"/"].concat();
candidates = candidates
.iter()
.filter_map(|c| {
if c.as_bytes().starts_with(&directory) {
Some(HgPath::new(&c.as_bytes()[directory.len()..]))
} else {
None
}
})
.collect();
}
// `self.dirs` includes all of the directories, recursively, so if
// we're attempting to match 'foo/bar/baz.txt', it'll have '', 'foo',
// 'foo/bar' in it. Thus we can safely ignore a candidate that has a
// '/' in it, indicating it's for a subdir-of-a-subdir; the immediate
// subdir will be in there without a slash.
VisitChildrenSet::Set(
candidates
.iter()
.filter_map(|c| {
if c.bytes().all(|b| *b != b'/') {
Some(*c)
} else {
None
}
})
.collect(),
)
} }
fn matches_everything(&self) -> bool { fn matches_everything(&self) -> bool {
false false
} }
fn is_exact(&self) -> bool { fn is_exact(&self) -> bool {
true true
} }
} }
#[cfg(test)]
mod tests {
use super::*;
use pretty_assertions::assert_eq;
#[test]
fn test_filematcher_visit_children_set() {
// Visitchildrenset
let files = vec![HgPath::new(b"dir/subdir/foo.txt")];
let matcher = FileMatcher::new(&files).unwrap();
let mut set = HashSet::new();
set.insert(HgPath::new(b"dir"));
assert_eq!(
matcher.visit_children_set(HgPath::new(b"")),
VisitChildrenSet::Set(set)
);
let mut set = HashSet::new();
set.insert(HgPath::new(b"subdir"));
assert_eq!(
matcher.visit_children_set(HgPath::new(b"dir")),
VisitChildrenSet::Set(set)
);
let mut set = HashSet::new();
set.insert(HgPath::new(b"foo.txt"));
assert_eq!(
matcher.visit_children_set(HgPath::new(b"dir/subdir")),
VisitChildrenSet::Set(set)
);
assert_eq!(
matcher.visit_children_set(HgPath::new(b"dir/subdir/x")),
VisitChildrenSet::Empty
);
assert_eq!(
matcher.visit_children_set(HgPath::new(b"dir/subdir/foo.txt")),
VisitChildrenSet::Empty
);
assert_eq!(
matcher.visit_children_set(HgPath::new(b"folder")),
VisitChildrenSet::Empty
);
}
#[test]
fn test_filematcher_visit_children_set_files_and_dirs() {
let files = vec![
HgPath::new(b"rootfile.txt"),
HgPath::new(b"a/file1.txt"),
HgPath::new(b"a/b/file2.txt"),
// No file in a/b/c
HgPath::new(b"a/b/c/d/file4.txt"),
];
let matcher = FileMatcher::new(&files).unwrap();
let mut set = HashSet::new();
set.insert(HgPath::new(b"a"));
set.insert(HgPath::new(b"rootfile.txt"));
assert_eq!(
matcher.visit_children_set(HgPath::new(b"")),
VisitChildrenSet::Set(set)
);
let mut set = HashSet::new();
set.insert(HgPath::new(b"b"));
set.insert(HgPath::new(b"file1.txt"));
assert_eq!(
matcher.visit_children_set(HgPath::new(b"a")),
VisitChildrenSet::Set(set)
);
let mut set = HashSet::new();
set.insert(HgPath::new(b"c"));
set.insert(HgPath::new(b"file2.txt"));
assert_eq!(
matcher.visit_children_set(HgPath::new(b"a/b")),
VisitChildrenSet::Set(set)
);
let mut set = HashSet::new();
set.insert(HgPath::new(b"d"));
assert_eq!(
matcher.visit_children_set(HgPath::new(b"a/b/c")),
VisitChildrenSet::Set(set)
);
let mut set = HashSet::new();
set.insert(HgPath::new(b"file4.txt"));
assert_eq!(
matcher.visit_children_set(HgPath::new(b"a/b/c/d")),
VisitChildrenSet::Set(set)
);
assert_eq!(
matcher.visit_children_set(HgPath::new(b"a/b/c/d/e")),
VisitChildrenSet::Empty
);
assert_eq!(
matcher.visit_children_set(HgPath::new(b"folder")),
VisitChildrenSet::Empty
);
}
}