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.

Download Raw Diff

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

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

Event Timeline

Alphare created this revision.Nov 29 2019, 1:22 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptNov 29 2019, 1:22 PM

Herald added subscribers: mercurial-devel, kevincox, durin42. · View Herald Transcript

Alphare added a child revision: D7530: rust-dirstate-status: update bridge for new rust version of `dirstate.status`.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

Alphare mentioned this in D7528: rust-matchers: add `FileMatcher` implementation.Dec 11 2019, 4:29 AM

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

			Path	Packages
M			rust/hg-core/src/dirstate/status.rs (116 lines)
M			rust/hg-cpython/src/dirstate/status.rs (17 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	18405		Nov 29 2019, 1:22 PM	★	★
Diff 2	18423		Dec 2 2019, 9:20 AM	★	★
Diff 3	18635		Dec 12 2019, 10:40 AM	★	★
Diff 4	18812	rHG8c77826116f7f59e49588667e2ee1b6fcd6be3be	Nov 29 2019, 11:29 AM	★	★

Commit	Parents	Author	Summary	Date
664c3f0673ad	9dd02062394b	Raphaël Gomès		Nov 29 2019, 11:29 AM

Status	Author	Revision
Closed	Alphare	D7531 rust-matchers: add support for `exactmatcher` in `dirstate.status`
Closed	Alphare	D7530 rust-dirstate-status: update bridge for new rust version of `dirstate.status`
Closed	Alphare	D7529 rust-dirstate-status: add `walk_explicit` implementation, use `Matcher` trait
Closed	Alphare	D7528 rust-matchers: add `FileMatcher` implementation
Closed	Alphare	D7527 rust-matchers: add doctests for `AlwaysMatcher`
Closed	Alphare	D7526 rust-hg-path: add method to get part of a path relative to a prefix
Closed	Alphare	D7525 rust-matchers: improve `Matcher` trait ergonomics
Closed	Alphare	D7524 rust-dirs-multiset: use `AsRef` instead of concrete types when possible
Closed	Alphare	D7523 rust-hg-path: implement `Display` for `HgPath` and `HgPathBuf`
Closed	Alphare	D7119 rust-dirstatemap: remove additional lookup in dirstate.matches
Abandoned	Alphare	D7118 rust-dirstatemap: remove additional lookups in traverse
Closed	Alphare	D7116 rust-performance: introduce FastHashMap type alias for HashMap

Diff 18635

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