This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7529

rust-dirstate-status: add `walk_explicit` implementation, use `Matcher` trait
ClosedPublic

Authored by Alphare on Nov 29 2019, 1:22 PM.

Details

Reviewers
kevincox
Group Reviewers
hg-reviewers
Commits
rHG8c77826116f7: rust-dirstate-status: add `walk_explicit` implementation, use `Matcher` trait
Summary

This is the first time we actually use the Matcher trait, still for a small
subset of all matchers defined in Python.

While I haven't yet actually measured the performance of this, I have tried
to avoid any unnecessary allocations. This forces the use of heavy lifetimes
annotations which I am not sure we can simplify, although I would be happy
to be proven wrong.

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 29 2019, 1:22 PM
kevincox requested changes to this revision.Dec 2 2019, 8:26 AM
kevincox added inline comments.
rust/hg-core/src/dirstate/status.rs
125

It seems to me that you can just use one lifetime here since the two input limit the output.

https://rust.godbolt.org/z/57bf6T But maybe I over-simplified the problem.

168

Isn't this the same as the first case? Can we just remove the first case?

240

It surprises me this annotation is required? I thought this would be the assumed meaning.

This revision now requires changes to proceed.Dec 2 2019, 8:26 AM
Alphare updated this revision to Diff 18423.Dec 2 2019, 9:20 AM
Alphare added inline comments.Dec 2 2019, 9:21 AM
rust/hg-core/src/dirstate/status.rs
125

You're right, the complex annotations are only needed on status, this only returns values from files.

168

The if will be needed in a (semi-distant) future series, I'll remove it for now, thanks.

240

I thought so too, but it seems that the compiler's elision rules do not apply with Iterators. It's probable a more complicated problem to solve in a deterministic fashion than just a plain Vec.

kevincox accepted this revision.Dec 2 2019, 10:16 AM
martinvonz added a subscriber: martinvonz.Dec 11 2019, 12:25 PM

This no longer applies cleanly (and doesn't build when applied with fuzz)

Alphare updated this revision to Diff 18635.Dec 12 2019, 10:40 AM
Alphare added a commit: rHG8c77826116f7: rust-dirstate-status: add `walk_explicit` implementation, use `Matcher` trait.Dec 17 2019, 9:49 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG8c77826116f7: rust-dirstate-status: add `walk_explicit` implementation, use `Matcher` trait (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 18812

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::{ use crate::{
dirstate::SIZE_FROM_OTHER_PARENT, dirstate::SIZE_FROM_OTHER_PARENT,
matchers::Matcher,
utils::{ utils::{
files::HgMetadata, files::HgMetadata,
hg_path::{hg_path_to_path_buf, HgPath}, hg_path::{hg_path_to_path_buf, HgPath},
}, },
CopyMap, DirstateEntry, DirstateMap, EntryState, CopyMap, DirstateEntry, DirstateMap, EntryState,
}; };
use rayon::prelude::*; use rayon::prelude::*;
use std::collections::HashSet;
use std::path::Path; use std::path::Path;
/// Marker enum used to dispatch new status entries into the right collections. /// Marker enum used to dispatch new status entries into the right collections.
/// Is similar to `crate::EntryState`, but represents the transient state of /// Is similar to `crate::EntryState`, but represents the transient state of
/// entries during the lifetime of a command. /// entries during the lifetime of a command.
enum Dispatch { enum Dispatch {
Unsure, Unsure,
Modified, Modified,
Added, Added,
Removed, Removed,
Deleted, Deleted,
Clean, Clean,
Unknown, Unknown,
} }
type IoResult<T> = std::io::Result<T>;
/// Dates and times that are outside the 31-bit signed range are compared /// Dates and times that are outside the 31-bit signed range are compared
/// modulo 2^31. This should prevent hg from behaving badly with very large /// modulo 2^31. This should prevent hg from behaving badly with very large
/// files or corrupt dates while still having a high probability of detecting /// files or corrupt dates while still having a high probability of detecting
/// changes. (issue2608) /// changes. (issue2608)
/// TODO I haven't found a way of having `b` be `Into<i32>`, since `From<u64>` /// TODO I haven't found a way of having `b` be `Into<i32>`, since `From<u64>`
/// is not defined for `i32`, and there is no `As` trait. This forces the /// is not defined for `i32`, and there is no `As` trait. This forces the
/// caller to cast `b` as `i32`. /// caller to cast `b` as `i32`.
fn mod_compare(a: i32, b: i32) -> bool { fn mod_compare(a: i32, b: i32) -> bool {
} }
// File was removed, everything is normal // File was removed, everything is normal
EntryState::Removed => Dispatch::Removed, EntryState::Removed => Dispatch::Removed,
// File is unknown to Mercurial, everything is normal // File is unknown to Mercurial, everything is normal
EntryState::Unknown => Dispatch::Unknown, EntryState::Unknown => Dispatch::Unknown,
} }
} }
/// Get stat data about the files explicitly specified by match.
/// TODO subrepos
fn walk_explicit<'a>(
kevincoxUnsubmitted
Not Done

It seems to me that you can just use one lifetime here since the two input limit the output.

https://rust.godbolt.org/z/57bf6T But maybe I over-simplified the problem.

kevincox: It seems to me that you can just use one lifetime here since the two input limit the output.
AlphareAuthorUnsubmitted
Done

You're right, the complex annotations are only needed on status, this only returns values from files.

Alphare: You're right, the complex annotations are only needed on `status`, this only returns values…
files: &'a HashSet<&HgPath>,
dmap: &'a DirstateMap,
root_dir: impl AsRef<Path> + Sync + Send,
check_exec: bool,
list_clean: bool,
last_normal_time: i64,
) -> impl ParallelIterator<Item = IoResult<(&'a HgPath, Dispatch)>> {
files.par_iter().filter_map(move |filename| {
// TODO normalization
let normalized = filename.as_ref();
let buf = match hg_path_to_path_buf(normalized) {
Ok(x) => x,
Err(e) => return Some(Err(e.into())),
};
let target = root_dir.as_ref().join(buf);
let st = target.symlink_metadata();
match st {
Ok(meta) => {
let file_type = meta.file_type();
if file_type.is_file() || file_type.is_symlink() {
if let Some(entry) = dmap.get(normalized) {
return Some(Ok((
normalized,
dispatch_found(
&normalized,
*entry,
HgMetadata::from_metadata(meta),
&dmap.copy_map,
check_exec,
list_clean,
last_normal_time,
),
)));
}
} else {
if dmap.contains_key(normalized) {
return Some(Ok((normalized, Dispatch::Removed)));
}
}
}
Err(_) => {
if let Some(entry) = dmap.get(normalized) {
kevincoxUnsubmitted
Not Done

Isn't this the same as the first case? Can we just remove the first case?

kevincox: Isn't this the same as the first case? Can we just remove the first case?
AlphareAuthorUnsubmitted
Done

The if will be needed in a (semi-distant) future series, I'll remove it for now, thanks.

Alphare: The `if` will be needed in a (semi-distant) future series, I'll remove it for now, thanks.
return Some(Ok((
normalized,
dispatch_missing(entry.state),
)));
}
}
};
None
})
}
/// Stat all entries in the `DirstateMap` and mark them for dispatch into /// Stat all entries in the `DirstateMap` and mark them for dispatch into
/// the relevant collections. /// the relevant collections.
fn stat_dmap_entries( fn stat_dmap_entries(
dmap: &DirstateMap, dmap: &DirstateMap,
root_dir: impl AsRef<Path> + Sync + Send, root_dir: impl AsRef<Path> + Sync + Send,
check_exec: bool, check_exec: bool,
list_clean: bool, list_clean: bool,
last_normal_time: i64, last_normal_time: i64,
) -> impl ParallelIterator<Item = std::io::Result<(&HgPath, Dispatch)>> { ) -> impl ParallelIterator<Item = IoResult<(&HgPath, Dispatch)>> {
dmap.par_iter().map(move |(filename, entry)| { dmap.par_iter().map(move |(filename, entry)| {
let filename: &HgPath = filename; let filename: &HgPath = filename;
let filename_as_path = hg_path_to_path_buf(filename)?; let filename_as_path = hg_path_to_path_buf(filename)?;
let meta = root_dir.as_ref().join(filename_as_path).symlink_metadata(); let meta = root_dir.as_ref().join(filename_as_path).symlink_metadata();
match meta { match meta {
Ok(ref m) Ok(ref m)
if !(m.file_type().is_file() if !(m.file_type().is_file()
pub added: Vec<&'a HgPath>, pub added: Vec<&'a HgPath>,
pub removed: Vec<&'a HgPath>, pub removed: Vec<&'a HgPath>,
pub deleted: Vec<&'a HgPath>, pub deleted: Vec<&'a HgPath>,
pub clean: Vec<&'a HgPath>, pub clean: Vec<&'a HgPath>,
/* TODO ignored /* TODO ignored
* TODO unknown */ * TODO unknown */
} }
fn build_response( fn build_response<'a>(
results: Vec<(&HgPath, Dispatch)>, results: impl IntoIterator<Item = IoResult<(&'a HgPath, Dispatch)>>,
) -> (Vec<&HgPath>, StatusResult) { ) -> IoResult<(Vec<&'a HgPath>, StatusResult<'a>)> {
kevincoxUnsubmitted
Not Done

It surprises me this annotation is required? I thought this would be the assumed meaning.

kevincox: It surprises me this annotation is required? I thought this would be the assumed meaning.
AlphareAuthorUnsubmitted
Done

I thought so too, but it seems that the compiler's elision rules do not apply with Iterators. It's probable a more complicated problem to solve in a deterministic fashion than just a plain Vec.

Alphare: I thought so too, but it seems that the compiler's elision rules do not apply with `Iterators`.
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, dispatch) in results.into_iter() { for res in results.into_iter() {
let (filename, dispatch) = res?;
match dispatch { match dispatch {
Dispatch::Unknown => {} Dispatch::Unknown => {}
Dispatch::Unsure => lookup.push(filename), Dispatch::Unsure => lookup.push(filename),
Dispatch::Modified => modified.push(filename), Dispatch::Modified => modified.push(filename),
Dispatch::Added => added.push(filename), Dispatch::Added => added.push(filename),
Dispatch::Removed => removed.push(filename), Dispatch::Removed => removed.push(filename),
Dispatch::Deleted => deleted.push(filename), Dispatch::Deleted => deleted.push(filename),
Dispatch::Clean => clean.push(filename), Dispatch::Clean => clean.push(filename),
} }
} }
( Ok((
lookup, lookup,
StatusResult { StatusResult {
modified, modified,
added, added,
removed, removed,
deleted, deleted,
clean, clean,
}, },
) ))
} }
pub fn status( pub fn status<'a: 'c, 'b: 'c, 'c>(
dmap: &DirstateMap, dmap: &'a DirstateMap,
matcher: &'b (impl Matcher),
root_dir: impl AsRef<Path> + Sync + Send + Copy, root_dir: impl AsRef<Path> + Sync + Send + 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<&HgPath>, StatusResult)> { ) -> IoResult<(Vec<&'c HgPath>, StatusResult<'c>)> {
let results: std::io::Result<_> = stat_dmap_entries( let files = matcher.file_set();
let mut results = vec![];
if let Some(files) = files {
results.par_extend(walk_explicit(
&files,
&dmap, &dmap,
root_dir, root_dir,
check_exec, check_exec,
list_clean, list_clean,
last_normal_time, last_normal_time,
) ));
.collect(); }
if !matcher.is_exact() {
let stat_results = stat_dmap_entries(
&dmap,
root_dir,
check_exec,
list_clean,
last_normal_time,
);
results.par_extend(stat_results);
}
Ok(build_response(results?)) build_response(results)
} }

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

use crate::dirstate::DirstateMap; use crate::dirstate::DirstateMap;
use cpython::exc::ValueError; use cpython::exc::ValueError;
use cpython::{ use cpython::{
PyBytes, PyErr, PyList, PyObject, PyResult, Python, PythonObject, PyBytes, PyErr, PyList, PyObject, PyResult, Python, PythonObject,
ToPyObject, ToPyObject,
}; };
use hg::utils::files::get_path_from_bytes; use hg::utils::files::get_path_from_bytes;
use hg::matchers::AlwaysMatcher;
use hg::status; use hg::status;
use hg::utils::hg_path::HgPath; use hg::utils::hg_path::HgPath;
/// This will be useless once trait impls for collection are added to `PyBytes` /// This will be useless once trait impls for collection are added to `PyBytes`
/// upstream. /// upstream.
fn collect_pybytes_list<P: AsRef<HgPath>>( fn collect_pybytes_list<P: AsRef<HgPath>>(
py: Python, py: Python,
collection: &[P], collection: &[P],
check_exec: bool, check_exec: bool,
) -> PyResult<(PyList, PyList, PyList, PyList, PyList, PyList, PyList)> { ) -> PyResult<(PyList, PyList, PyList, PyList, PyList, PyList, PyList)> {
let bytes = root_dir.extract::<PyBytes>(py)?; let bytes = root_dir.extract::<PyBytes>(py)?;
let root_dir = get_path_from_bytes(bytes.data(py)); let root_dir = get_path_from_bytes(bytes.data(py));
let dmap: DirstateMap = dmap.to_py_object(py); let dmap: DirstateMap = dmap.to_py_object(py);
let dmap = dmap.get_inner(py); let dmap = dmap.get_inner(py);
let (lookup, status_res) = // TODO removed in the next patch to get the code to compile. This patch
status(&dmap, &root_dir, list_clean, last_normal_time, check_exec) // is part of a series and does not make real sense on its own.
let matcher = AlwaysMatcher;
let (lookup, status_res) = status(
&dmap,
&matcher,
&root_dir,
list_clean,
last_normal_time,
check_exec,
)
.map_err(|e| PyErr::new::<ValueError, _>(py, e.to_string()))?; .map_err(|e| PyErr::new::<ValueError, _>(py, e.to_string()))?;
let modified = collect_pybytes_list(py, status_res.modified.as_ref()); let modified = collect_pybytes_list(py, status_res.modified.as_ref());
let added = collect_pybytes_list(py, status_res.added.as_ref()); let added = collect_pybytes_list(py, status_res.added.as_ref());
let removed = collect_pybytes_list(py, status_res.removed.as_ref()); let removed = collect_pybytes_list(py, status_res.removed.as_ref());
let deleted = collect_pybytes_list(py, status_res.deleted.as_ref()); let deleted = collect_pybytes_list(py, status_res.deleted.as_ref());
let clean = collect_pybytes_list(py, status_res.clean.as_ref()); let clean = collect_pybytes_list(py, status_res.clean.as_ref());
let lookup = collect_pybytes_list(py, lookup.as_ref()); let lookup = collect_pybytes_list(py, lookup.as_ref());
let unknown = PyList::new(py, &[]); let unknown = PyList::new(py, &[]);
Ok((lookup, modified, added, removed, deleted, unknown, clean)) Ok((lookup, modified, added, removed, deleted, unknown, clean))
} }