This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6147

discovery: move cl.hasnode outside of the for-loop
ClosedPublic

Authored by pulkit on Mar 17 2019, 11:49 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG19ccc6788a27: discovery: move cl.hasnode outside of the for-loop
Summary

IIUC, resolving attributes for changelog can lead to some overhead. So this
patch moves that to outside of a for-loop.

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

pulkit created this revision.Mar 17 2019, 11:49 AM
lothiraldan added a subscriber: lothiraldan.Mar 18 2019, 5:28 AM

The series looks good to me after a very quick review. I think a more in-deep review will be necessary.

@pulkit Do you have measured some performance improvement with this series?

pulkit added a comment.Mar 18 2019, 7:30 AM
In D6147#89602, @lothiraldan wrote:

The series looks good to me after a very quick review. I think a more in-deep review will be necessary.
@pulkit Do you have measured some performance improvement with this series?

I didn't. I am not sure whether this will have any significant performance benefit. I will try to have some numbers.

pulkit added a comment.Mar 19 2019, 7:21 AM

@lothiraldan There is no significant performance benefit which I am able to notice. There is minor improvement of around 0.2 sec on our large repo but that is not stable.

I was reviewing one of the patches when I saw this code and found it needs some cleanup and did that.

Closed by commit rHG19ccc6788a27: discovery: move cl.hasnode outside of the for-loop (authored by pulkit). · Explain WhyMar 27 2019, 3:37 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/discovery.py (2 lines)

Diff 14602

mercurial/discovery.py

for n in outgoing.missing: for n in outgoing.missing:
ctx = repo[n] ctx = repo[n]
missingctx.add(ctx) missingctx.add(ctx)
branches.add(ctx.branch()) branches.add(ctx.branch())
with remote.commandexecutor() as e: with remote.commandexecutor() as e:
remotemap = e.callcommand('branchmap', {}).result() remotemap = e.callcommand('branchmap', {}).result()
knownnode = cl.hasnode # do not use nodemap until it is filtered
# A. register remote heads of branches which are in outgoing set # A. register remote heads of branches which are in outgoing set
for branch, heads in remotemap.iteritems(): for branch, heads in remotemap.iteritems():
# don't add head info about branches which we don't have locally # don't add head info about branches which we don't have locally
if branch not in branches: if branch not in branches:
continue continue
known = [] known = []
unsynced = [] unsynced = []
knownnode = cl.hasnode # do not use nodemap until it is filtered
for h in heads: for h in heads:
if knownnode(h): if knownnode(h):
known.append(h) known.append(h)
else: else:
unsynced.append(h) unsynced.append(h)
headssum[branch] = (known, list(known), unsynced) headssum[branch] = (known, list(known), unsynced)
# B. add new branch data # B. add new branch data