Page MenuHomePhabricator

rust: itering less on MissingAncestors.bases for max()

Authored by gracinet on Feb 12 2019, 7:02 AM.



Instead of iterating on the whole self.bases each time to find
its max, we keep the latter in a separate member attribute and
keep it up to date in add_bases()

On a perfdiscovery done on PyPy, with repos prepared with
contrib/ 50 100, this gives a slight
improvement (around 0.5% on wall time, but 10% on CPU)

! wall 0.172801 comb 0.180000 user 0.180000 sys 0.000000 (median of 541)
! wall 0.171798 comb 0.160000 user 0.160000 sys 0.000000 (median of 551)

(perf command run time upped because of bigger variability during this test).

Diff Detail

rHG Mercurial
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gracinet created this revision.Feb 12 2019, 7:02 AM
kevincox accepted this revision.Feb 12 2019, 9:58 AM
kevincox added inline comments.

Does it make sense to just default this to -1 and remove the option?

gracinet updated this revision to Diff 14046.Feb 12 2019, 11:27 AM
gracinet added inline comments.Feb 12 2019, 11:32 AM

I must confess to have hesitated a bit on that one. On one hand, it would work, but semantically if ever a new "special" revision -2 is introduced, this could become problematic. On the other hand, there are probably a few areas that already depend on the fact that NULL_REVISION is the smallest one.

From the graph side of things, NULL_REVISION being a common ancestor of all revisions, so it must be smaller or equal than all, so it doesn't matter much.

Option<Revision> was merely a simple way to be sure not to be wrong, I'm open to suggestions.

kevincox added inline comments.Feb 12 2019, 5:20 PM

If you don't want to depend on the value of NULL_REVISION you can use the minimum value for the type backing revision.

Other special revisions shouldn't be relevant because you shouldn't be comparing them as the max anyways.

That being said the currents state is fine. So if you think it makes more sense that sounds good to me.

gracinet added inline comments.Feb 13 2019, 5:18 AM

Ok, I've made up my mind: whatever the representation, NULL_REVISION must be smaller than all other Revision. Using it, rather than -1will indeed make this patch simpler.

Also, in subsequent (not yet submitted) works, I'm already liberally using this fact that NULL_REVISION is the smallest of all, so…

gracinet updated this revision to Diff 14057.Feb 13 2019, 6:55 AM
This revision was automatically updated to reflect the committed changes.
kevincox added inline comments.Feb 16 2019, 1:37 AM

You are doing an empty check and an unwrap. I think it would be nice to combine these to make it obvious that there is no panic chance here.

let max_revs = revs_visit.iter().cloned().max();
let max_revs = match max_revs {
  Some(max) => max,
  None => return Ok(Vec::new()),