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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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 25862

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