This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7254

rust-status: improve status performance
ClosedPublic

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

Details

Reviewers
kevincox
Group Reviewers
hg-reviewers
Commits
rHG889ac87e8bfd: rust-status: improve status performance
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.Nov 6 2019, 8:22 AM
kevincox accepted this revision.Nov 6 2019, 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.Nov 7 2019, 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.Nov 8 2019, 7:51 AM
Alphare added a commit: rHG889ac87e8bfd: rust-status: improve status performance.Nov 11 2019, 4:08 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG889ac87e8bfd: rust-status: improve status performance (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/dirstate/status.rs (241 lines)

Diff 18017

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

// status.rs // status.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Rust implementation of dirstate.status (dirstate.py). //! Rust implementation of dirstate.status (dirstate.py).
//! It is currently missing a lot of functionality compared to the Python one //! It is currently missing a lot of functionality compared to the Python one
//! and will only be triggered in narrow cases. //! and will only be triggered in narrow cases.
use crate::utils::files::HgMetadata; use crate::utils::files::HgMetadata;
use crate::utils::hg_path::{hg_path_to_path_buf, HgPathBuf}; use crate::utils::hg_path::{hg_path_to_path_buf, HgPath};
use crate::{DirstateEntry, DirstateMap, EntryState}; use crate::{CopyMap, DirstateEntry, DirstateMap, EntryState};
use rayon::prelude::*; use rayon::prelude::*;
use std::path::Path; use std::path::Path;
// Stat all entries in the `DirstateMap` and return their new metadata. /// Marker enum used to dispatch new status entries into the right collections.
pub fn stat_dmap_entries( /// Is similar to `crate::EntryState`, but represents the transient state of
/// entries during the lifetime of a command.
enum Dispatch {
Unsure,
Modified,
Added,
Removed,
Deleted,
Clean,
Unknown,
}
/// The file corresponding to the dirstate entry was found on the filesystem.
fn dispatch_found(
filename: impl AsRef<HgPath>,
entry: DirstateEntry,
metadata: HgMetadata,
copy_map: &CopyMap,
check_exec: bool,
list_clean: bool,
last_normal_time: i64,
) -> Dispatch {
let DirstateEntry {
kevincoxUnsubmitted
Done

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.

kevincox: Every exit path is just `filename.to_owned()`. It would be better to just let the caller do…
state,
mode,
mtime,
size,
} = entry;
let HgMetadata {
st_mode,
st_size,
st_mtime,
..
} = metadata;
match state {
EntryState::Normal => {
// Dates and times that are outside the 31-bit signed
// range are compared modulo 2^31. This should prevent
// it from behaving badly with very large files or
// corrupt dates while still having a high probability
// of detecting changes. (issue2608)
let range_mask = 0x7fffffff;
let size_changed = (size != st_size as i32)
kevincoxUnsubmitted
Not Done

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()
kevincox: I would create a helper function. ``` fn mod_compare(a: i32, b: impl Into<i32>) -> bool {…
AlphareAuthorUnsubmitted
Done

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

Alphare: See inline comment further in the series about how I went about this issue.
&& size != (st_size as i32 & range_mask);
let mode_changed =
(mode ^ st_mode as i32) & 0o100 != 0o000 && check_exec;
if size >= 0
&& (size_changed || mode_changed)
|| size == -2 // other parent
|| copy_map.contains_key(filename.as_ref())
kevincoxUnsubmitted
Done

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.

kevincox: I would break this out into another local. ``` let metadata_changed = size >= 0 &&…
{
kevincoxUnsubmitted
Done

Can we put this magic number in a constant anywhere?

kevincox: Can we put this magic number in a constant anywhere?
Dispatch::Modified
} else if mtime != st_mtime as i32
&& mtime != (st_mtime as i32 & range_mask)
{
Dispatch::Unsure
} else if st_mtime == last_normal_time {
// the file may have just been marked as normal and
// it may have changed in the same second without
// changing its size. This can happen if we quickly
// do multiple commits. Force lookup, so we don't
// miss such a racy file change.
Dispatch::Unsure
} else if list_clean {
Dispatch::Clean
} else {
Dispatch::Unknown
}
}
EntryState::Merged => Dispatch::Modified,
EntryState::Added => Dispatch::Added,
EntryState::Removed => Dispatch::Removed,
EntryState::Unknown => Dispatch::Unknown,
}
}
/// The file corresponding to this Dirstate entry is missing.
fn dispatch_missing(state: EntryState) -> Dispatch {
match state {
// File was removed from the filesystem during commands
EntryState::Normal | EntryState::Merged | EntryState::Added => {
Dispatch::Deleted
}
// File was removed, everything is normal
kevincoxUnsubmitted
Done

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

kevincox: Same, we don't need to return the path.
EntryState::Removed => Dispatch::Removed,
// File is unknown to Mercurial, everything is normal
EntryState::Unknown => Dispatch::Unknown,
}
}
/// Stat all entries in the `DirstateMap` and mark them for dispatch into
/// the relevant collections.
fn stat_dmap_entries(
dmap: &DirstateMap, dmap: &DirstateMap,
root_dir: impl AsRef<Path> + Sync, root_dir: impl AsRef<Path> + Sync,
) -> std::io::Result<Vec<(HgPathBuf, Option<HgMetadata>)>> { check_exec: bool,
list_clean: bool,
last_normal_time: i64,
) -> std::io::Result<Vec<(&HgPath, Dispatch)>> {
kevincoxUnsubmitted
Done

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.

kevincox: It would be best to return the iterator here, this way we don't need to collect into a `Vec`…
dmap.par_iter() dmap.par_iter()
.filter_map( .filter_map(
// Getting file metadata is costly, so we don't do it if the // Getting file metadata is costly, so we don't do it if the
// file is already present in the results, hence `filter_map` // file is already present in the results, hence `filter_map`
|(filename, _)| -> Option< |(filename, entry)| -> Option<
std::io::Result<(HgPathBuf, Option<HgMetadata>)> std::io::Result<(&HgPath, Dispatch)>
kevincoxUnsubmitted
Done

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

kevincox: Unless I am missing something this function always returns `Some(_)`.
> { > {
let meta = match hg_path_to_path_buf(filename) { let meta = match hg_path_to_path_buf(filename) {
Ok(p) => root_dir.as_ref().join(p).symlink_metadata(), Ok(p) => root_dir.as_ref().join(p).symlink_metadata(),
Err(e) => return Some(Err(e.into())), Err(e) => return Some(Err(e.into())),
}; };
Some(match meta { Some(match meta {
Ok(ref m) Ok(ref m)
if !(m.file_type().is_file() if !(m.file_type().is_file()
|| m.file_type().is_symlink()) => || m.file_type().is_symlink()) =>
{ {
Ok((filename.to_owned(), None)) Ok((filename, dispatch_missing(entry.state)))
} }
Ok(m) => Ok(( Ok(m) => Ok((filename, dispatch_found(
filename.to_owned(), filename,
Some(HgMetadata::from_metadata(m)), *entry,
)), HgMetadata::from_metadata(m),
&dmap.copy_map,
check_exec,
list_clean,
last_normal_time))),
Err(ref e) Err(ref e)
if e.kind() == std::io::ErrorKind::NotFound if e.kind() == std::io::ErrorKind::NotFound
|| e.raw_os_error() == Some(20) => || e.raw_os_error() == Some(20) =>
{ {
// Rust does not yet have an `ErrorKind` for // Rust does not yet have an `ErrorKind` for
// `NotADirectory` (errno 20) // `NotADirectory` (errno 20)
// It happens if the dirstate contains `foo/bar` and // It happens if the dirstate contains `foo/bar` and
// foo is not a directory // foo is not a directory
Ok((filename.to_owned(), None)) Ok((filename, dispatch_missing(entry.state)))
} }
Err(e) => Err(e), Err(e) => Err(e),
}) })
}, },
) )
.collect() .collect()
} }
pub struct StatusResult { pub struct StatusResult<'a> {
pub modified: Vec<HgPathBuf>, pub modified: Vec<&'a HgPath>,
pub added: Vec<HgPathBuf>, pub added: Vec<&'a HgPath>,
pub removed: Vec<HgPathBuf>, pub removed: Vec<&'a HgPath>,
pub deleted: Vec<HgPathBuf>, pub deleted: Vec<&'a HgPath>,
pub clean: Vec<HgPathBuf>, pub clean: Vec<&'a HgPath>,
// TODO ignored // TODO ignored
// TODO unknown // TODO unknown
} }
fn build_response( fn build_response(
dmap: &DirstateMap, results: Vec<(&HgPath, Dispatch)>,
list_clean: bool, ) -> (Vec<&HgPath>, StatusResult) {
last_normal_time: i64,
check_exec: bool,
results: Vec<(HgPathBuf, Option<HgMetadata>)>,
) -> (Vec<HgPathBuf>, StatusResult) {
let mut lookup = vec![]; let mut lookup = vec![];
let mut modified = vec![]; let mut modified = vec![];
let mut added = vec![]; let mut added = vec![];
let mut removed = vec![]; let mut removed = vec![];
let mut deleted = vec![]; let mut deleted = vec![];
let mut clean = vec![]; let mut clean = vec![];
for (filename, metadata_option) in results.into_iter() { for (filename, dispatch) in results.into_iter() {
let DirstateEntry { match dispatch {
state, Dispatch::Unknown => {}
mode, Dispatch::Unsure => lookup.push(filename),
mtime, Dispatch::Modified => modified.push(filename),
size, Dispatch::Added => added.push(filename),
} = match dmap.get(&filename) { Dispatch::Removed => removed.push(filename),
None => { Dispatch::Deleted => deleted.push(filename),
continue; Dispatch::Clean => clean.push(filename),
}
Some(e) => *e,
};
match metadata_option {
None => {
match state {
EntryState::Normal
| EntryState::Merged
| EntryState::Added => deleted.push(filename),
EntryState::Removed => removed.push(filename),
_ => {}
};
}
Some(HgMetadata {
st_mode,
st_size,
st_mtime,
..
}) => {
match state {
EntryState::Normal => {
// Dates and times that are outside the 31-bit signed
// range are compared modulo 2^31. This should prevent
// it from behaving badly with very large files or
// corrupt dates while still having a high probability
// of detecting changes. (issue2608)
let range_mask = 0x7fffffff;
let size_changed = (size != st_size as i32)
&& size != (st_size as i32 & range_mask);
let mode_changed = (mode ^ st_mode as i32) & 0o100
!= 0o000
&& check_exec;
if size >= 0
&& (size_changed || mode_changed)
|| size == -2 // other parent
|| dmap.copy_map.contains_key(&filename)
{
modified.push(filename);
} else if mtime != st_mtime as i32
&& mtime != (st_mtime as i32 & range_mask)
{
lookup.push(filename);
} else if st_mtime == last_normal_time {
// the file may have just been marked as normal and
// it may have changed in the same second without
// changing its size. This can happen if we quickly
// do multiple commits. Force lookup, so we don't
// miss such a racy file change.
lookup.push(filename);
} else if list_clean {
clean.push(filename);
}
}
EntryState::Merged => modified.push(filename),
EntryState::Added => added.push(filename),
EntryState::Removed => removed.push(filename),
EntryState::Unknown => {}
}
}
} }
} }
( (
lookup, lookup,
StatusResult { StatusResult {
modified, modified,
added, added,
removed, removed,
deleted, deleted,
clean, clean,
}, },
) )
} }
pub fn status( pub fn status(
dmap: &DirstateMap, dmap: &DirstateMap,
root_dir: impl AsRef<Path> + Sync + Copy, root_dir: impl AsRef<Path> + Sync + Copy,
list_clean: bool, list_clean: bool,
last_normal_time: i64, last_normal_time: i64,
check_exec: bool, check_exec: bool,
) -> std::io::Result<(Vec<HgPathBuf>, StatusResult)> { ) -> std::io::Result<(Vec<&HgPath>, StatusResult)> {
let results = stat_dmap_entries(&dmap, root_dir)?; let results = stat_dmap_entries(
Ok(build_response(
&dmap, &dmap,
root_dir,
check_exec,
list_clean, list_clean,
last_normal_time, last_normal_time,
check_exec, )?;
results,
)) Ok(build_response(results))
} }