This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8585

scmutil: speed up relativization of paths when it's a no-op
ClosedPublic

Authored by valentin.gatienbaron on May 26 2020, 7:19 AM.

Details

Reviewers
marmoute
durin42
Group Reviewers
hg-reviewers
Commits
rHG5d8ae9248a70: scmutil: speed up relativization of paths when it's a no-op
Summary

ie when calling hg from the root of a repository. This is a common
thing to do in tools, so worth optimizing. Running hg files > /tmp/a
from the root of mozilla-central on linux:

before:
real 0m1,510s
user 0m1,387s
sys 0m0,090s

after:
real 0m1,266s
user 0m1,165s
sys 0m0,073s

(there are 280k paths, so relativization was costing ~1us per path
somehow)

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

joerg.sonnenberger added a subscriber: joerg.sonnenberger.May 26 2020, 7:39 AM

I find the comment confusing. Maybe something like Relative paths below the top-level of the repository need more processing.?

valentin.gatienbaron added a comment.May 26 2020, 8:46 AM

What's confusing about it? For me, the emphasis is that the slow branch defines the expected behavior, and the other branch is merely an optimization of it.

marmoute accepted this revision.May 26 2020, 10:01 AM
marmoute added a subscriber: marmoute.

I don't find the comment confusing, (but I am not agains clarification if @joerg.sonnenberger and @valentin.gatienbaron reach a consensus.

joerg.sonnenberger added a comment.May 26 2020, 10:10 AM

At the very least, there is missing a word like "also" or "still", but it doesn't really tell the *why*. That's what would make a comment here useful, IMO. I.e. the important point is that a repo path is a relative path from the repo root, so if the cwd is empty, no additional work is necessary.

durin42 accepted this revision.May 26 2020, 12:16 PM
durin42 added a subscriber: durin42.

I can see some room for improvement in the log message, but it's good enough IMO and I can't really figure out a better wording.

This revision is now accepted and ready to land.May 26 2020, 12:16 PM
valentin.gatienbaron added a comment.EditedMay 26 2020, 12:45 PM

I reworded the description/comment slightly while Augie accepted the revision, in case people care enough to want the modified commit queued.

joerg.sonnenberger added a comment.May 26 2020, 3:34 PM

Yes, that version is better.

Revision Contents
Changeset List

PathPackages
Mmercurial/scmutil.py (9 lines)

Diff 21481

mercurial/scmutil.py

relative = stringutil.parsebool(config) relative = stringutil.parsebool(config)
if relative is None: if relative is None:
raise error.ConfigError( raise error.ConfigError(
_(b"ui.relative-paths is not a boolean ('%s')") % config _(b"ui.relative-paths is not a boolean ('%s')") % config
) )
if relative: if relative:
cwd = repo.getcwd() cwd = repo.getcwd()
if cwd != b'':
# this branch is correct when cwd == b'', ie cwd = repo root,
# but it's slower
pathto = repo.pathto pathto = repo.pathto
return lambda f: pathto(f, cwd) return lambda f: pathto(f, cwd)
elif repo.ui.configbool(b'ui', b'slash'): if repo.ui.configbool(b'ui', b'slash'):
return lambda f: f return lambda f: f
else: else:
return util.localpath return util.localpath
def subdiruipathfn(subpath, uipathfn): def subdiruipathfn(subpath, uipathfn):
'''Create a new uipathfn that treats the file as relative to subpath.''' '''Create a new uipathfn that treats the file as relative to subpath.'''
return lambda f: uipathfn(posixpath.join(subpath, f)) return lambda f: uipathfn(posixpath.join(subpath, f))