Page MenuHomePhabricator

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

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

Details

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
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 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.

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

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
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.