( )⚙ D4326 setdiscovery: precompute children revisions to avoid quadratic lookup

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4326

setdiscovery: precompute children revisions to avoid quadratic lookup
ClosedPublic

Authored by indygreg on Aug 17 2018, 5:31 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG274acf379dbb: setdiscovery: precompute children revisions to avoid quadratic lookup
Summary

Moving away from dagutil a few commits ago introduced quadratic
behavior when resolving children revisions during discovery.

This commit introduces a precompute step of the children revisions
to avoid the bad behavior.

I believe the new code should have near identical performance to
what dagutil was doing before. Behavior is still slightly different
because we take into account filtered revisions. But this change was
made when we moved off dagutil.

I added a comment about multiple invocations of this function
redundantly calculating the children revisions. I believe this
potentially undesirable behavior was present when we used dagutil,
as the call to inverse() previously in this function created a new
object and required computing children on every invocation. I thought
we should document the potential for a performance issue rather than
let it go undocumented.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Aug 17 2018, 5:31 PM
yuja added a subscriber: yuja.Aug 18 2018, 4:05 AM

+ parentrevs = repo.changelog.parentrevs
+ for rev in repo.changelog.revs(start=min(revsroots)):
+ # Always ensure revision has an entry so we don't need to worry about
+ # missing keys.
+ children.setdefault(rev, [])
+
+ for prev in parentrevs(rev):
+ if prev == nullrev:
+ continue
+
+ children.setdefault(prev, []).append(rev)

Just FYI, I remember I wrote a similar code, which is a53bfc2845f2.

Closed by commit rHG274acf379dbb: setdiscovery: precompute children revisions to avoid quadratic lookup (authored by indygreg). · Explain WhyAug 18 2018, 4:07 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/setdiscovery.py (26 lines)

Diff 10420

mercurial/setdiscovery.py

return sample return sample
def _takefullsample(repo, headrevs, revs, size): def _takefullsample(repo, headrevs, revs, size):
sample = set(repo.revs('heads(%ld)', revs)) sample = set(repo.revs('heads(%ld)', revs))
# update from heads # update from heads
revsheads = set(repo.revs('heads(%ld)', revs)) revsheads = set(repo.revs('heads(%ld)', revs))
_updatesample(revs, revsheads, sample, repo.changelog.parentrevs) _updatesample(revs, revsheads, sample, repo.changelog.parentrevs)
# update from roots # update from roots
revsroots = set(repo.revs('roots(%ld)', revs)) revsroots = set(repo.revs('roots(%ld)', revs))
# TODO this is quadratic # _updatesample() essentially does interaction over revisions to look up
parentfn = lambda rev: repo.changelog.children(repo.changelog.node(rev)) # their children. This lookup is expensive and doing it in a loop is
# quadratic. We precompute the children for all relevant revisions and
# make the lookup in _updatesample() a simple dict lookup.
#
# Because this function can be called multiple times during discovery, we
# may still perform redundant work and there is room to optimize this by
# keeping a persistent cache of children across invocations.
children = {}
parentrevs = repo.changelog.parentrevs
for rev in repo.changelog.revs(start=min(revsroots)):
# Always ensure revision has an entry so we don't need to worry about
# missing keys.
children.setdefault(rev, [])
for prev in parentrevs(rev):
if prev == nullrev:
continue
children.setdefault(prev, []).append(rev)
_updatesample(revs, revsroots, sample, parentfn) _updatesample(revs, revsroots, sample, children.__getitem__)
assert sample assert sample
sample = _limitsample(sample, size) sample = _limitsample(sample, size)
if len(sample) < size: if len(sample) < size:
more = size - len(sample) more = size - len(sample)
sample.update(random.sample(list(revs - sample), more)) sample.update(random.sample(list(revs - sample), more))
return sample return sample
def _limitsample(sample, desiredlen): def _limitsample(sample, desiredlen):