This is an archive of the discontinued Mercurial Phabricator instance.

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

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



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

Diff Detail

rHG Mercurial
No Linters Available
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 inline comments.

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

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

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

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

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

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 Any update on this patch?
Note: I will get to your remarks on the first change of the stack very soon as well.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.