This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8586

files: speed up `hg files` when no flags change display
ClosedPublic

Authored by valentin.gatienbaron on May 26 2020, 8:37 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHG065421e12248: files: speed up `hg files` when no flags change display
Summary

It's not the first time I see slowness from this command slow down
tools built on top of hg.

The majority of the time is spent merely printing the result before
this change, which is clearly not how it should be (especially since
the computation of the result also looks slow).

Running hg files in mozilla-central:

parent revision: 1,260s
this commit: 0,683s
this commit without batching ui.write: 0,931s
this commit replacing the body of the loop with pass: 0,566s

This looks like a prime candidate for a rust fast path, but until
then, it seems reasonable to optimize the python.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

joerg.sonnenberger added a subscriber: joerg.sonnenberger.May 26 2020, 8:57 AM
joerg.sonnenberger added inline comments.
mercurial/cmdutil.py
2760

The normal idiom for batches uses a list and a single join at time of writing. Not sure if there are repositories where we care about the size of the temporary buffer enough to flush it in the middle.

marmoute requested changes to this revision.May 26 2020, 10:04 AM
marmoute added a subscriber: marmoute.

The overall idea seems good, but @joerg.sonnenberger feedback sound probably be taken in account.

mercurial/cmdutil.py
2760

+1 we should use a list here, there are optimisation around such string concatenation but I would not could on it.

This revision now requires changes to proceed.May 26 2020, 10:04 AM
valentin.gatienbaron marked 2 inline comments as done.May 26 2020, 12:24 PM
valentin.gatienbaron added inline comments.
mercurial/cmdutil.py
2760

Using a list is fine (and in fact, it has the advantage of reusing memory). I tried one big join at the end but it's slightly slower, and it make hg files | head -n 1 100ms slower, probably because the iterator gets fully evaluated.

durin42 added a subscriber: durin42.May 26 2020, 12:24 PM

Doesn't this break HGPLAIN=1 hg files -T'{path|short}\n' or similar?

valentin.gatienbaron added a comment.May 26 2020, 12:29 PM
In D8586#128112, @durin42 wrote:

Doesn't this break HGPLAIN=1 hg files -T'{path|short}\n' or similar?

I think you're thinking of ui.plain. fm.isplain() implies no template.

marmoute accepted this revision.May 26 2020, 12:37 PM
valentin.gatienbaron added a commit: rHG065421e12248: files: speed up `hg files` when no flags change display.May 28 2020, 11:43 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG065421e12248: files: speed up `hg files` when no flags change display (authored by valentin.gatienbaron). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/cmdutil.py (31 lines)

Diff 21480

mercurial/cmdutil.py

forgot.extend(f for f in forget if f not in rejected) forgot.extend(f for f in forget if f not in rejected)
return bad, forgot return bad, forgot
def files(ui, ctx, m, uipathfn, fm, fmt, subrepos): def files(ui, ctx, m, uipathfn, fm, fmt, subrepos):
ret = 1 ret = 1
needsfctx = ui.verbose or {b'size', b'flags'} & fm.datahint() needsfctx = ui.verbose or {b'size', b'flags'} & fm.datahint()
if fm.isplain() and not needsfctx:
# Fast path. The speed-up comes from skipping the formatter, and batching
# calls to ui.write.
buf = []
for f in ctx.matches(m):
buf.append(fmt % uipathfn(f))
joerg.sonnenbergerUnsubmitted
Done

The normal idiom for batches uses a list and a single join at time of writing. Not sure if there are repositories where we care about the size of the temporary buffer enough to flush it in the middle.

joerg.sonnenberger: The normal idiom for batches uses a list and a single join at time of writing. Not sure if…
marmouteUnsubmitted
Done

+1 we should use a list here, there are optimisation around such string concatenation but I would not could on it.

marmoute: +1 we should use a list here, there are optimisation around such string concatenation but I…
valentin.gatienbaronAuthorUnsubmitted
Done

Using a list is fine (and in fact, it has the advantage of reusing memory). I tried one big join at the end but it's slightly slower, and it make hg files | head -n 1 100ms slower, probably because the iterator gets fully evaluated.

valentin.gatienbaron: Using a list is fine (and in fact, it has the advantage of reusing memory). I tried one big…
if len(buf) > 100:
ui.write(b''.join(buf))
del buf[:]
ret = 0
if buf:
ui.write(b''.join(buf))
else:
for f in ctx.matches(m): for f in ctx.matches(m):
fm.startitem() fm.startitem()
fm.context(ctx=ctx) fm.context(ctx=ctx)
if needsfctx: if needsfctx:
fc = ctx[f] fc = ctx[f]
fm.write(b'size flags', b'% 10d % 1s ', fc.size(), fc.flags()) fm.write(b'size flags', b'% 10d % 1s ', fc.size(), fc.flags())
fm.data(path=f) fm.data(path=f)
fm.plain(fmt % uipathfn(f)) fm.plain(fmt % uipathfn(f))
ret = 0 ret = 0
for subpath in sorted(ctx.substate): for subpath in sorted(ctx.substate):
submatch = matchmod.subdirmatcher(subpath, m) submatch = matchmod.subdirmatcher(subpath, m)
subuipathfn = scmutil.subdiruipathfn(subpath, uipathfn) subuipathfn = scmutil.subdiruipathfn(subpath, uipathfn)
if subrepos or m.exact(subpath) or any(submatch.files()): if subrepos or m.exact(subpath) or any(submatch.files()):
sub = ctx.sub(subpath) sub = ctx.sub(subpath)
try: try:
recurse = m.exact(subpath) or subrepos recurse = m.exact(subpath) or subrepos