This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9650

copies-rust: process copy information of both parent at the same time
ClosedPublic

Authored by marmoute on Dec 21 2020, 5:28 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG600f8d510ab6: copies-rust: process copy information of both parent at the same time
rHG10f85c26b35a: copies-rust: process copy information of both parent at the same time
Summary

This avoid a double iteration and this open the way to a better handing of
deletion. That better handling of deletion is the core reason we are doing this
refactoring.

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

marmoute created this revision.Dec 21 2020, 5:28 PM
marmoute updated this revision to Diff 24489.Dec 22 2020, 9:12 PM
baymax updated this revision to Diff 24500.Dec 22 2020, 9:46 PM

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

baymax updated this revision to Diff 24584.Jan 5 2021, 4:07 AM

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

Alphare requested changes to this revision.Jan 8 2021, 8:48 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/copy_tracing.rs
472

I'd rather use a if let (None, None) = (&base_p1_copies, &base_p2_copies) , but it's not a big deal.

477

This is let mut p1_copies = base_p1_copies.clone();

487

Looks like you're doing a clone that isn't needed unless we're in the ::Removed case and then unwrapping.

This revision now requires changes to proceed.Jan 8 2021, 8:48 AM
marmoute added inline comments.Jan 8 2021, 9:41 AM
rust/hg-core/src/copy_tracing.rs
472

m'okay

477

sweet

487

The clone is needed for both remove and Copied Case. We could try to be smarter, only cloning at the first edit, but the underlying data are im-rs:ordmap so cloning is cheap.

Alphare added inline comments.Jan 8 2021, 10:08 AM
rust/hg-core/src/copy_tracing.rs
487

You're right, I'm blind. :)

Alphare accepted this revision.Jan 8 2021, 10:44 AM
This revision now requires review to proceed.Jan 8 2021, 10:44 AM
baymax updated this revision to Diff 24675.Jan 8 2021, 6:16 PM

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

baymax updated this revision to Diff 25695.Feb 22 2021, 9:14 AM

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

baymax updated this revision to Diff 25754.Feb 22 2021, 11:21 AM

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

baymax updated this revision to Diff 25793.Feb 22 2021, 5:12 PM

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

marmoute added a commit: rHG10f85c26b35a: copies-rust: process copy information of both parent at the same time.Feb 24 2021, 11:02 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG10f85c26b35a: copies-rust: process copy information of both parent at the same time (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/copy_tracing.rs (93 lines)

Diff 25854

rust/hg-core/src/copy_tracing.rs

/// ChangedFiles struct it return to this attribute. The DataHolder /// ChangedFiles struct it return to this attribute. The DataHolder
/// lifetime will be at least as long as the ChangedFiles one. /// lifetime will be at least as long as the ChangedFiles one.
pub data: Option<D>, pub data: Option<D>,
} }
pub type RevInfoMaker<'a, D> = pub type RevInfoMaker<'a, D> =
Box<dyn for<'r> Fn(Revision, &'r mut DataHolder<D>) -> RevInfo<'r> + 'a>; Box<dyn for<'r> Fn(Revision, &'r mut DataHolder<D>) -> RevInfo<'r> + 'a>;
/// enum used to carry information about the parent → child currently processed
#[derive(Copy, Clone, Debug)]
enum Parent {
/// The `p1(x) → x` edge
FirstParent,
/// The `p2(x) → x` edge
SecondParent,
}
/// 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>,
for rev in revs { for rev in revs {
let mut d: DataHolder<D> = DataHolder { data: None }; let mut d: DataHolder<D> = DataHolder { data: None };
let (p1, p2, changes) = rev_info(rev, &mut d); let (p1, p2, changes) = rev_info(rev, &mut d);
// We will chain the copies information accumulated for the parent with // We will chain the copies information accumulated for the parent with
// the individual copies information the curent revision. Creating a // the individual copies information the curent revision. Creating a
// new TimeStampedPath for each `rev` → `children` vertex. // new TimeStampedPath for each `rev` → `children` vertex.
let mut copies: Option<InternalPathCopies> = None;
// 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 all_copies,
&mut children_count, &mut 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 all_copies,
&mut children_count, &mut 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 = match p1_copies { let (p1_copies, p2_copies) =
None => None, chain_changes(&mut path_map, p1_copies, p2_copies, &changes, rev);
Some(parent_copies) => Some(add_from_changes(
&mut path_map,
&parent_copies,
&changes,
Parent::FirstParent,
rev,
)),
};
let p2_copies = match p2_copies {
None => None,
Some(parent_copies) => Some(add_from_changes(
&mut path_map,
&parent_copies,
&changes,
Parent::SecondParent,
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, &path_map, rev, p2_copies, p1_copies, &changes,
)), )),
}; };
Some(c) => Some(c.clone()), Some(c) => Some(c.clone()),
None => Some(InternalPathCopies::default()), None => Some(InternalPathCopies::default()),
} }
} }
} }
/// Combine ChangedFiles with some existing PathCopies information and return /// Combine ChangedFiles with some existing PathCopies information and return
/// the result /// the result
fn add_from_changes( fn chain_changes(
path_map: &mut TwoWayPathMap, path_map: &mut TwoWayPathMap,
base_copies: &InternalPathCopies, base_p1_copies: Option<InternalPathCopies>,
base_p2_copies: Option<InternalPathCopies>,
changes: &ChangedFiles, changes: &ChangedFiles,
parent: Parent,
current_rev: Revision, current_rev: Revision,
) -> InternalPathCopies { ) -> (Option<InternalPathCopies>, Option<InternalPathCopies>) {
let mut copies = base_copies.clone(); // Fast path the "nothing to do" case.
if let (None, None) = (&base_p1_copies, &base_p2_copies) {
AlphareUnsubmitted
Not Done

I'd rather use a if let (None, None) = (&base_p1_copies, &base_p2_copies) , but it's not a big deal.

Alphare: I'd rather use a `if let (None, None) = (&base_p1_copies, &base_p2_copies)` , but it's not a…
marmouteAuthorUnsubmitted
Done

m'okay

marmoute: m'okay
return (None, None);
}
let mut p1_copies = base_p1_copies.clone();
let mut p2_copies = base_p2_copies.clone();
AlphareUnsubmitted
Not Done

This is let mut p1_copies = base_p1_copies.clone();

Alphare: This is `let mut p1_copies = base_p1_copies.clone();`
marmouteAuthorUnsubmitted
Done

sweet

marmoute: sweet
for action in changes.iter_actions() { for action in changes.iter_actions() {
match action { match action {
Action::CopiedFromP1(path_dest, path_source) => { Action::CopiedFromP1(path_dest, path_source) => {
match parent { match &mut p1_copies {
_ => (), // not the parent we are looking for None => (), // This is not a vertex we should proceed.
Parent::FirstParent => add_one_copy( Some(copies) => add_one_copy(
current_rev, current_rev,
path_map, path_map,
&mut copies, copies,
&base_copies, base_p1_copies.as_ref().unwrap(),
AlphareUnsubmitted
Not Done

Looks like you're doing a clone that isn't needed unless we're in the ::Removed case and then unwrapping.

Alphare: Looks like you're doing a clone that isn't needed unless we're in the `::Removed` case and then…
marmouteAuthorUnsubmitted
Done

The clone is needed for both remove and Copied Case. We could try to be smarter, only cloning at the first edit, but the underlying data are im-rs:ordmap so cloning is cheap.

marmoute: The clone is needed for both remove and Copied Case. We could try to be smarter, only cloning…
AlphareUnsubmitted
Not Done

You're right, I'm blind. :)

Alphare: You're right, I'm blind. :)
path_dest, path_dest,
path_source, path_source,
), ),
}; }
} }
Action::CopiedFromP2(path_dest, path_source) => { Action::CopiedFromP2(path_dest, path_source) => {
match parent { match &mut p2_copies {
_ => (), //not the parent we are looking for None => (), // This is not a vertex we should proceed.
Parent::SecondParent => add_one_copy( Some(copies) => add_one_copy(
current_rev, current_rev,
path_map, path_map,
&mut copies, copies,
&base_copies, base_p2_copies.as_ref().unwrap(),
path_dest, path_dest,
path_source, path_source,
), ),
}; }
} }
Action::Removed(deleted_path) => { Action::Removed(deleted_path) => {
// We must drop copy information for removed file. // We must drop copy information for removed file.
// //
// We need to explicitly record them as dropped to // We need to explicitly record them as dropped to
// propagate this information when merging two // propagate this information when merging two
// InternalPathCopies object. // InternalPathCopies object.
let deleted = path_map.tokenize(deleted_path); let deleted = path_map.tokenize(deleted_path);
match &mut p1_copies {
None => (),
Some(copies) => {
copies.entry(deleted).and_modify(|old| { copies.entry(deleted).and_modify(|old| {
old.mark_delete(current_rev); old.mark_delete(current_rev);
}); });
} }
};
match &mut p2_copies {
None => (),
Some(copies) => {
copies.entry(deleted).and_modify(|old| {
old.mark_delete(current_rev);
});
}
};
}
} }
} }
copies (p1_copies, p2_copies)
} }
// insert one new copy information in an InternalPathCopies // insert one new copy information in an InternalPathCopies
// //
// This deal with chaining and overwrite. // This deal with chaining and overwrite.
fn add_one_copy( fn add_one_copy(
current_rev: Revision, current_rev: Revision,
path_map: &mut TwoWayPathMap, path_map: &mut TwoWayPathMap,