This is an archive of the discontinued Mercurial Phabricator instance.

rust-discovery: takefullsample() core implementation
ClosedPublic

Authored by gracinet on Apr 17 2019, 8:17 AM.

Details

Summary

take_full_sample() browses the undecided set in both directions: from
its roots as well as from its heads.

Following what's done on the Python side, we alter update_sample()
signature to take a closure returning an iterator: either ParentsIterator
or an iterator over the children found in children_cache. These constructs
should probably be split off in a separate module.

This is a first concrete example where a more abstract graph notion (probably
a trait) would be useful, as this is nothing but an operation on the reversed
DAG.

A similar motivation in the context of the discovery
process would be to replace the call to dagops::range in
add_missing_revisions() with a simple iteration over descendents, again an
operation on the reversed graph.

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

gracinet created this revision.Apr 17 2019, 8:17 AM

As I've tried to make clear in the changeset description, this is still work in progress.

That being said, I wanted to put in on display because:

  • together with its descendants, it leads to a drop-in replacement for the Python partialdiscovery object
  • it's a good support for feedback on the question of more abstract DAG representations
  • the children cache leads to the same design problem as the undecided member attributed had (see D6231). I expect to get more across similar issues while translating Python code

Thanks !

kevincox accepted this revision.Apr 17 2019, 8:57 AM
kevincox added inline comments.
rust/hg-core/src/discovery.rs
107

You might also consider an enum

enum Parents {
  Zero,
  One(Revision),
  Two(Revision, Revision),
}

This also makes the iterator implementation a little clearer.

136

Can't you just call self.next() in both cases?

273

Generally I would just do this.

for &rev in self.undecided.as_ref().unwrap() {
274

Same.

gracinet updated this revision to Diff 15022.May 6 2019, 11:30 AM
gracinet marked an inline comment as done.May 6 2019, 11:38 AM

I think it's maybe ready to land in that form. In the future, I'd like to put this ParentsIterator in a more generic place, and IMHO, it should become part of an AbstractGraph trait, that could live in a graph module.

Ideally, we should even be able to generically reverse these graphs, so that the children iteration that's been done in this discovery process would just be the same, as seen from update_sample, but it's maybe too much asking.

rust/hg-core/src/discovery.rs
136

Yes that's what I actually did first, and then I tried this variant to see if it's faster. It may be a bit, but nothing worth doing in a first inclusion.

273

yes, same as in parent commit. Indeed it's a nicer way to iterate on references to Copy objects

274

for this one, I can actually use ParentsIterator, now, spares me the NULL_REVISION check

gracinet marked an inline comment as done.May 16 2019, 12:43 PM
This revision was automatically updated to reflect the committed changes.