Page MenuHomePhabricator

rust-status: improve status performance
ClosedPublic

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

Details

Summary

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

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.Wed, Nov 6, 8:22 AM
kevincox accepted this revision.Wed, Nov 6, 11:22 AM
kevincox added inline comments.
rust/hg-core/src/dirstate/status.rs
41

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.

64

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()
71

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.

72

Can we put this magic number in a constant anywhere?

105

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

120

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.

126

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

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

Thanks for the review @kevincox.

rust/hg-core/src/dirstate/status.rs
64

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

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