This is an archive of the discontinued Mercurial Phabricator instance.

rust-status: improve status performance

Authored by Alphare on Nov 6 2019, 8:22 AM.



This change does more things in the parallel loop, refactors the file-level
logic into two functions for added clarity.

This bit of Rust code takes 55ms to execute on a repo where the stat'ing part
of Valentin's fast path takes 40ms. While the code differs a bit and it's hard
to get an exact measurement of how much of a performance impact it has, I can
be fairly certain that this implementation is *at worse* twice as slow.

Diff Detail

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

Event Timeline

Alphare created this revision.Nov 6 2019, 8:22 AM
kevincox accepted this revision.Nov 6 2019, 11:22 AM
kevincox added inline comments.

Every exit path is just filename.to_owned(). It would be better to just let the caller do that if required and return only the Dispatch.


I would create a helper function.

fn mod_compare(a: i32, b: impl Into<i32>) -> bool {
  let b  = b.into();
  a != b && a != b & range_mask

Also I think the condition is easier to read as:

a & i32::max_value() != b & i32::max_value()

I would break this out into another local.

let metadata_changed = size >= 0 && (size_changed || mode_changed);
let other_parent = size == -2;
if metadata_change || other_parent || copy_map.contains_key(filename)

As a bonus this also makes the &&/|| precedence obvious.


Can we put this magic number in a constant anywhere?


Same, we don't need to return the path.


It would be best to return the iterator here, this way we don't need to collect into a Vec just to throw it away.

You can do the final sorting using a parallel for_each. However even if we end up doing a collection into Vec we can do that at the place of use rather than here which forces it upon all callers.


Unless I am missing something this function always returns Some(_).

Alphare marked 7 inline comments as done.Nov 7 2019, 5:29 AM

Thanks for the review @kevincox.


See inline comment further in the series about how I went about this issue.

Alphare updated this revision to Diff 17739.Nov 8 2019, 7:51 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.