This is an archive of the discontinued Mercurial Phabricator instance.

rust-status: add bare `hg status` support in hg-core
ClosedPublic

Authored by Alphare on Jan 17 2020, 11:53 AM.

Details

Summary

A lot of performance remains to be gained, most notably by doing more things
in parallel, but also by caching, not falling back to Python but switching
to another regex engine, etc..

I have measured on multiple repositories that this change, when in combination
with the next two patches, improve bare hg status performance, and has no
observable impact when falling back (because it does so early).

On the Netbeans repository:
C: 840ms
Rust+C: 556ms

Mozilla Central with the one pattern that causes a fallback removed:
C: 2.315s
Rust+C: 1.700 s

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Jan 17 2020, 11:53 AM

I did a first pass on this. Nothing obviously wrong, but I am bit out of my Rust league here. I asked a couple of question.

rust/hg-core/src/dirstate/status.rs
29–40

I am curious about where this changes comes from? did rust-format changed opinion or did we reached a threshold?

277

Does this TODO affect the "readiness" of this code to be used in production ? If not, can you clarify what that TODO implies ?

691

What's the implication of this comment ? (or rephrase, the consequence on not having that "done" yet?)

Alphare added inline comments.Feb 6 2020, 10:13 AM
rust/hg-core/src/dirstate/status.rs
29–40

I was fixing conflicts and prefer this style. rustfmt does not nest already separate imports. If that's an issue I suppose I can change it.

277

Clarified in the updated inline comment. I mainly wanted to avoid even more code that had very little chance of being useful. If you disagree, please let me know.

691

Oops, it's "insensitive", not "sensitive".

Alphare updated this revision to Diff 19947.Feb 6 2020, 10:16 AM
marmoute added inline comments.Feb 6 2020, 2:42 PM
rust/hg-core/src/dirstate/status.rs
29–40

This is not an issue, but this add noise that distract from the core of the patch. Consider doing this kind og small change in independant changesets.

277

It is still not very clear to me. Do we have BadType case where it would make sense to use somethign else than Unknown?

691

So, right now, the status code do now work on case insensitive file system. I assume the proper gards are in place at a higher level ?

Alphare marked an inline comment as done.Feb 10 2020, 11:27 AM
Alphare added inline comments.
rust/hg-core/src/dirstate/status.rs
277

We could check for block devices, character devices, etc. depending on the platform.

691

Yes, there is a check before the fast-path.

Alphare marked an inline comment as done.Feb 10 2020, 11:30 AM
Alphare updated this revision to Diff 20048.
Alphare updated this revision to Diff 20157.Feb 11 2020, 5:55 AM
kevincox requested changes to this revision.Feb 12 2020, 5:27 AM
kevincox added inline comments.
rust/hg-core/src/dirstate/status.rs
205

Please describe this parameter.

272

This seems like something that should be handled in the caller. It is trivial for the caller to separate files from directories if they wish to do so.

274

I believe these two can be replaced with .flatten()

464

Why was this changed when IIUC all of the returns are references? If a caller wants that I would prefer to do a map in the caller.

626

tx and rx are bad names, please indicate what is being sent and recieved.

627

Instead of creating a vec then extending use collect.

628

You are first collecting all of results, then collecting all of work. It looks like you intended to do this in parallel but I don't think that is what is happening.

A better approach is to make walk_explicit return both items and partition them here.

This revision now requires changes to proceed.Feb 12 2020, 5:27 AM
Alphare marked an inline comment as done.Feb 12 2020, 9:08 AM
Alphare added inline comments.
rust/hg-core/src/dirstate/status.rs
464

traverse returns some Cow::Owned, I felt that it was simpler to share the same return signature for all functions, but I can map the output, I guess.

kevincox added inline comments.Feb 12 2020, 11:24 AM
rust/hg-core/src/dirstate/status.rs
464

I can definitely see that this is a judgement call. However .map(Cow::Borrowed) is easy enough to write that I think it makes sense in this case. However both ways are acceptable.

Alphare marked 5 inline comments as done.Feb 13 2020, 12:37 PM
Alphare updated this revision to Diff 20188.
Alphare added inline comments.Feb 14 2020, 4:30 AM
rust/hg-core/src/dirstate/status.rs
464

I think your way is cleaner, so let's do that, also for walk_explicit.

Alphare updated this revision to Diff 20454.Mar 4 2020, 10:08 AM
Alphare updated this revision to Diff 20462.Mar 4 2020, 11:55 AM
Alphare updated this revision to Diff 20497.Mar 5 2020, 3:33 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.