D7254: rust-status: improve status performance

[kevincox (Kevin Cox)]

kevincox added inline comments.
kevincox accepted this revision.

INLINE COMMENTS

> status.rs:41
> +    last_normal_time: i64,
> +) -> (HgPathBuf, Dispatch) {
> +    let filename = filename.as_ref();

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`.

> status.rs:64
> +            // of detecting changes. (issue2608)
> +            let range_mask = 0x7fffffff;
> +

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

> status.rs:71
> +            if size >= 0
> +                            && (size_changed || mode_changed)
> +                            || size == -2  // other parent

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.

> status.rs:72
> +                            && (size_changed || mode_changed)
> +                            || size == -2  // other parent
> +                            || copy_map.contains_key(filename)

Can we put this magic number in a constant anywhere?

> status.rs:105
> +    state: EntryState,
> +) -> (HgPathBuf, Dispatch) {
> +    let filename = filename.as_ref();

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

> status.rs:127
> +    last_normal_time: i64,
> +) -> std::io::Result<Vec<(HgPathBuf, Dispatch)>> {
>      dmap.par_iter()

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.

> status.rs:133
> +            |(filename, entry)| -> Option<
> +                std::io::Result<(HgPathBuf, Dispatch)>
>              > {

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7254/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7254

To: Alphare, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel