This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9683

copies-rust: split up combine_changeset_copies function into a struct
ClosedPublic

Authored by SimonSapin on Jan 6 2021, 9:12 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHGcb4b0b0c6de4: copies-rust: split up combine_changeset_copies function into a struct
rHGcc1270f5c121: copies-rust: split up combine_changeset_copies function into a struct
Summary

… such that each iteration of its former loop is now a method call,
with the caller driving the loop.

This entirely removes the need for the DataHolder hack:
the method now takes a ChangedFiles<'_> parameter that borrows
a bytes buffer that can be owned by the caller’s stack frame,
just for the duration of that call.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

SimonSapin created this revision.Jan 6 2021, 9:12 AM
Alphare accepted this revision.Jan 22 2021, 4:48 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/copy_tracing.rs
380–381

This should be a docstring, same goes for finish

baymax updated this revision to Diff 25703.Feb 22 2021, 9:25 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 25740.Feb 22 2021, 11:00 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 25785.Feb 22 2021, 3:47 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

SimonSapin added a commit: rHGcc1270f5c121: copies-rust: split up combine_changeset_copies function into a struct.Feb 24 2021, 11:03 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGcc1270f5c121: copies-rust: split up combine_changeset_copies function into a struct (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 25703

rust/hg-core/src/copy_tracing.rs

} }
self.rev == other.rev self.rev == other.rev
} }
} }
/// maps CopyDestination to Copy Source (+ a "timestamp" for the operation) /// maps CopyDestination to Copy Source (+ a "timestamp" for the operation)
type InternalPathCopies = OrdMap<PathToken, CopySource>; type InternalPathCopies = OrdMap<PathToken, CopySource>;
/// hold parent 1, parent 2 and relevant files actions.
pub type RevInfo<'a> = (Revision, Revision, ChangedFiles<'a>);
/// represent the files affected by a changesets /// represent the files affected by a changesets
/// ///
/// This hold a subset of mercurial.metadata.ChangingFiles as we do not need /// This hold a subset of mercurial.metadata.ChangingFiles as we do not need
/// all the data categories tracked by it. /// all the data categories tracked by it.
/// This hold a subset of mercurial.metadata.ChangingFiles as we do not need /// This hold a subset of mercurial.metadata.ChangingFiles as we do not need
/// all the data categories tracked by it. /// all the data categories tracked by it.
pub struct ChangedFiles<'a> { pub struct ChangedFiles<'a> {
nb_items: u32, nb_items: u32,
} else if copy == P2_COPY { } else if copy == P2_COPY {
return Some(Action::CopiedFromP2(file, source)); return Some(Action::CopiedFromP2(file, source));
} }
} }
return None; return None;
} }
} }
/// A small struct whose purpose is to ensure lifetime of bytes referenced in
/// ChangedFiles
///
/// It is passed to the RevInfoMaker callback who can assign any necessary
/// content to the `data` attribute. The copy tracing code is responsible for
/// keeping the DataHolder alive at least as long as the ChangedFiles object.
pub struct DataHolder<D> {
/// RevInfoMaker callback should assign data referenced by the
/// ChangedFiles struct it return to this attribute. The DataHolder
/// lifetime will be at least as long as the ChangedFiles one.
pub data: Option<D>,
}
pub type RevInfoMaker<'a, D> =
Box<dyn for<'r> Fn(Revision, &'r mut DataHolder<D>) -> RevInfo<'r> + 'a>;
/// A small "tokenizer" responsible of turning full HgPath into lighter /// A small "tokenizer" responsible of turning full HgPath into lighter
/// PathToken /// PathToken
/// ///
/// Dealing with small object, like integer is much faster, so HgPath input are /// Dealing with small object, like integer is much faster, so HgPath input are
/// turned into integer "PathToken" and converted back in the end. /// turned into integer "PathToken" and converted back in the end.
#[derive(Clone, Debug, Default)] #[derive(Clone, Debug, Default)]
struct TwoWayPathMap { struct TwoWayPathMap {
token: HashMap<HgPathBuf, PathToken>, token: HashMap<HgPathBuf, PathToken>,
fn untokenize(&self, token: PathToken) -> &HgPathBuf { fn untokenize(&self, token: PathToken) -> &HgPathBuf {
assert!(token < self.path.len(), format!("Unknown token: {}", token)); assert!(token < self.path.len(), format!("Unknown token: {}", token));
&self.path[token] &self.path[token]
} }
} }
/// Same as mercurial.copies._combine_changeset_copies, but in Rust. /// Same as mercurial.copies._combine_changeset_copies, but in Rust.
/// pub struct CombineChangesetCopies {
/// Arguments are: all_copies: HashMap<Revision, InternalPathCopies>,
/// path_map: TwoWayPathMap,
/// revs: all revisions to be considered children_count: HashMap<Revision, usize>,
/// children: a {parent ? [childrens]} mapping }
/// target_rev: the final revision we are combining copies to
/// rev_info(rev): callback to get revision information: impl CombineChangesetCopies {
/// * first parent pub fn new(children_count: HashMap<Revision, usize>) -> Self {
/// * second parent Self {
/// * ChangedFiles all_copies: HashMap::new(),
/// isancestors(low_rev, high_rev): callback to check if a revision is an path_map: TwoWayPathMap::default(),
/// ancestor of another children_count,
pub fn combine_changeset_copies<D>( }
revs: Vec<Revision>, }
mut children_count: HashMap<Revision, usize>,
target_rev: Revision, /// Combined the given `changes` data specific to `rev` with the data
AlphareUnsubmitted
Not Done

This should be a docstring, same goes for finish

Alphare: This should be a docstring, same goes for `finish`
rev_info: RevInfoMaker<D>, /// previously given for its parents (and transitively, its ancestors).
) -> PathCopies { pub fn add_revision(
let mut all_copies = HashMap::new(); &mut self,
rev: Revision,
let mut path_map = TwoWayPathMap::default(); p1: Revision,
p2: Revision,
for rev in revs { changes: ChangedFiles<'_>,
let mut d: DataHolder<D> = DataHolder { data: None }; ) {
let (p1, p2, changes) = rev_info(rev, &mut d);
// We will chain the copies information accumulated for the parent with
// the individual copies information the curent revision. Creating a
// new TimeStampedPath for each `rev` → `children` vertex.
// Retrieve data computed in a previous iteration // Retrieve data computed in a previous iteration
let p1_copies = match p1 { let p1_copies = match p1 {
NULL_REVISION => None, NULL_REVISION => None,
_ => get_and_clean_parent_copies( _ => get_and_clean_parent_copies(
&mut all_copies, &mut self.all_copies,
&mut children_count, &mut self.children_count,
p1, p1,
), // will be None if the vertex is not to be traversed ), // will be None if the vertex is not to be traversed
}; };
let p2_copies = match p2 { let p2_copies = match p2 {
NULL_REVISION => None, NULL_REVISION => None,
_ => get_and_clean_parent_copies( _ => get_and_clean_parent_copies(
&mut all_copies, &mut self.all_copies,
&mut children_count, &mut self.children_count,
p2, p2,
), // will be None if the vertex is not to be traversed ), // will be None if the vertex is not to be traversed
}; };
// combine it with data for that revision // combine it with data for that revision
let (p1_copies, p2_copies) = let (p1_copies, p2_copies) = chain_changes(
chain_changes(&mut path_map, p1_copies, p2_copies, &changes, rev); &mut self.path_map,
p1_copies,
p2_copies,
&changes,
rev,
);
let copies = match (p1_copies, p2_copies) { let copies = match (p1_copies, p2_copies) {
(None, None) => None, (None, None) => None,
(c, None) => c, (c, None) => c,
(None, c) => c, (None, c) => c,
(Some(p1_copies), Some(p2_copies)) => Some(merge_copies_dict( (Some(p1_copies), Some(p2_copies)) => Some(merge_copies_dict(
&path_map, rev, p2_copies, p1_copies, &changes, &self.path_map,
rev,
p2_copies,
p1_copies,
&changes,
)), )),
}; };
if let Some(c) = copies { if let Some(c) = copies {
all_copies.insert(rev, c); self.all_copies.insert(rev, c);
} }
} }
// Drop internal information (like the timestamp) and return the final /// Drop intermediate data (such as which revision a copy was from) and
// mapping. /// return the final mapping.
let tt_result = all_copies pub fn finish(mut self, target_rev: Revision) -> PathCopies {
let tt_result = self
.all_copies
.remove(&target_rev) .remove(&target_rev)
.expect("target revision was not processed"); .expect("target revision was not processed");
let mut result = PathCopies::default(); let mut result = PathCopies::default();
for (dest, tt_source) in tt_result { for (dest, tt_source) in tt_result {
if let Some(path) = tt_source.path { if let Some(path) = tt_source.path {
let path_dest = path_map.untokenize(dest).to_owned(); let path_dest = self.path_map.untokenize(dest).to_owned();
let path_path = path_map.untokenize(path).to_owned(); let path_path = self.path_map.untokenize(path).to_owned();
result.insert(path_dest, path_path); result.insert(path_dest, path_path);
} }
} }
result result
} }
}
/// fetch previous computed information /// fetch previous computed information
/// ///
/// If no other children are expected to need this information, we drop it from /// If no other children are expected to need this information, we drop it from
/// the cache. /// the cache.
/// ///
/// If parent is not part of the set we are expected to walk, return None. /// If parent is not part of the set we are expected to walk, return None.
fn get_and_clean_parent_copies( fn get_and_clean_parent_copies(

rust/hg-cpython/src/copy_tracing.rs

use cpython::ObjectProtocol; use cpython::ObjectProtocol;
use cpython::PyBytes; use cpython::PyBytes;
use cpython::PyDict; use cpython::PyDict;
use cpython::PyList; use cpython::PyList;
use cpython::PyModule; use cpython::PyModule;
use cpython::PyObject; use cpython::PyObject;
use cpython::PyResult; use cpython::PyResult;
use cpython::PyTuple; use cpython::PyTuple;
use cpython::Python; use cpython::Python;
use hg::copy_tracing::combine_changeset_copies;
use hg::copy_tracing::ChangedFiles; use hg::copy_tracing::ChangedFiles;
use hg::copy_tracing::DataHolder; use hg::copy_tracing::CombineChangesetCopies;
use hg::copy_tracing::RevInfo;
use hg::copy_tracing::RevInfoMaker;
use hg::Revision; use hg::Revision;
/// Combines copies information contained into revision `revs` to build a copy /// Combines copies information contained into revision `revs` to build a copy
/// map. /// map.
/// ///
/// See mercurial/copies.py for details /// See mercurial/copies.py for details
pub fn combine_changeset_copies_wrapper( pub fn combine_changeset_copies_wrapper(
py: Python, py: Python,
revs: PyList, revs: PyList,
children_count: PyDict, children_count: PyDict,
target_rev: Revision, target_rev: Revision,
rev_info: PyObject, rev_info: PyObject,
) -> PyResult<PyDict> { ) -> PyResult<PyDict> {
let revs: PyResult<_> = let children_count = children_count
revs.iter(py).map(|r| Ok(r.extract(py)?)).collect();
// Wrap the `rev_info_maker` python callback as a Rust closure
//
// No errors are expected from the Python side, and they will should only
// happens in case of programing error or severe data corruption. Such
// errors will raise panic and the rust-cpython harness will turn them into
// Python exception.
let rev_info_maker: RevInfoMaker<PyBytes> =
Box::new(|rev: Revision, d: &mut DataHolder<PyBytes>| -> RevInfo {
let res: PyTuple = rev_info
.call(py, (rev,), None)
.expect("rust-copy-tracing: python call to `rev_info` failed")
.cast_into(py)
.expect(
"rust-copy_tracing: python call to `rev_info` returned \
unexpected non-Tuple value",
);
let p1 = res.get_item(py, 0).extract(py).expect(
"rust-copy-tracing: rev_info return is invalid, first item \
is a not a revision",
);
let p2 = res.get_item(py, 1).extract(py).expect(
"rust-copy-tracing: rev_info return is invalid, first item \
is a not a revision",
);
let files = match res.get_item(py, 2).extract::<PyBytes>(py) {
Ok(raw) => {
// Give responsability for the raw bytes lifetime to
// hg-core
d.data = Some(raw);
let addrs = d.data.as_ref().expect(
"rust-copy-tracing: failed to get a reference to the \
raw bytes for copy data").data(py);
ChangedFiles::new(addrs)
}
// value was presumably None, meaning they was no copy data.
Err(_) => ChangedFiles::new_empty(),
};
(p1, p2, files)
});
let children_count: PyResult<_> = children_count
.items(py) .items(py)
.iter() .iter()
.map(|(k, v)| Ok((k.extract(py)?, v.extract(py)?))) .map(|(k, v)| Ok((k.extract(py)?, v.extract(py)?)))
.collect(); .collect::<PyResult<_>>()?;
/// (Revision number, parent 1, parent 2, copy data for this revision)
type RevInfo = (Revision, Revision, Revision, Option<PyBytes>);
let res = combine_changeset_copies( let revs_info = revs.iter(py).map(|rev_py| -> PyResult<RevInfo> {
revs?, let rev = rev_py.extract(py)?;
children_count?, let tuple: PyTuple =
target_rev, rev_info.call(py, (rev_py,), None)?.cast_into(py)?;
rev_info_maker, let p1 = tuple.get_item(py, 0).extract(py)?;
); let p2 = tuple.get_item(py, 1).extract(py)?;
let opt_bytes = tuple.get_item(py, 2).extract(py)?;
Ok((rev, p1, p2, opt_bytes))
});
let mut combine_changeset_copies =
CombineChangesetCopies::new(children_count);
for rev_info in revs_info {
let (rev, p1, p2, opt_bytes) = rev_info?;
let files = match &opt_bytes {
Some(bytes) => ChangedFiles::new(bytes.data(py)),
// value was presumably None, meaning they was no copy data.
None => ChangedFiles::new_empty(),
};
combine_changeset_copies.add_revision(rev, p1, p2, files)
}
let path_copies = combine_changeset_copies.finish(target_rev);
let out = PyDict::new(py); let out = PyDict::new(py);
for (dest, source) in res.into_iter() { for (dest, source) in path_copies.into_iter() {
out.set_item( out.set_item(
py, py,
PyBytes::new(py, &dest.into_vec()), PyBytes::new(py, &dest.into_vec()),
PyBytes::new(py, &source.into_vec()), PyBytes::new(py, &source.into_vec()),
)?; )?;
} }
Ok(out) Ok(out)
} }