1567617 - Rebasing in mercurial takes forever with format-source enabled

Status: RESOLVED INVALID

(Developer Infrastructure :: Lint and Formatting, defect, P3)

Description

[Markus Stange [:mstange]]

Attachment: hg-flamegraph3.svg

I frequently rebase long stacks of patches, and I've noticed a large regression in the times these rebases take recently. I recently updated my mercurial version and then enabled the format-source extension.

I took a python profile using py-spy (see the attached SVG) and it shows that all the time is spent in this call to old_ctx.status in format-source/init.py:

def touched(repo, old_ctx, new_ctx, paths):
    matcher = rootedmatch(repo, new_ctx, paths)
    if any(path in new_ctx for path in paths):
        status = old_ctx.status(other=new_ctx, match=matcher)
        return bool(status.modified or status.added)
    return False

While I was filing this bug, I spent at least 20 minutes waiting to rebase something like 6 or 8 patches via a histedit roll operation.

Comment 1

[Victor Porof [:vporof][:vp]]

This is expected, and format-source is intended only for temporary use.

We plan to turn format-source off by default a month after formatting m-c, and this would end up being August 5th. The assumption being that by that time, enough people rebased their patches to the new coding style so a default configuration isn't necessary anymore. When it is, it can be turned on manually on a case-by-case basis with an .hgrc configuration.

In the meantime, if you don't need automatic patch rebasing anymore, remove the corresponding line which enables the extension in your .hgrc config file.

Comment 2

[Axel Hecht [:Pike]]

Reading through the code based on the profile, it seems that https://hg.mozilla.org/hgcustom/version-control-tools/file/709c897f2444dcfda27725af607cdf2cfade5d0d/hgext/format-source/__init__.py#l343 is hit for each line in https://hg.mozilla.org/mozilla-central/file/tip/.hg-format-source, and sees if any of the configpaths for that line have changed.

I.e., we're testing [".clang-format", ".clang-format-ignore"] some 50 times, and [".eslintrc.js", ".eslintignore", ".prettierrc", ".prettierignore"] some 40 times?

Connor, did I read that right? Sounds like that could be optimized by caching that result?

Comment 3

[:Gijs (he/him)]

(In reply to Victor Porof [:vporof][:vp] from comment #1)

This is expected, and format-source is intended only for temporary use.

I'm confused. Isn't format-source also what enforces that style mistakes are fixed on commit? How do I preserve that behaviour while turning this off? And wouldn't it be better if we taught format-source to ignore rebases that don't cross the mass reformatting cset(s), rather than relying on another mach bootstrap update to turn it off for everyone again?

Comment 4

[Victor Porof [:vporof][:vp]]

The format-source hg extension handles automatic patch formatting (generalized to work for any formatter, in our case for C++ and JS through clang-format and prettier respectively). The clang-format hg extension formats c++ at commit time. The js-format hg extension formats (and lints) js at commit time. These are all separate and can be individually turned on or off.

It might be better to make format-source smarter, however I'm unconvinced that it's worth pouring engineering effort into. I don't expect these kinds of tree-wide formatting changes to happen often. However it is certainly true we might want to standardize coding style for more languages.

Comment 5

[:Gijs (he/him)]

(In reply to Victor Porof [:vporof][:vp] from comment #4)

The format-source hg extension handles automatic patch formatting (generalized to work for any formatter, in our case for C++ and JS through clang-format and prettier respectively). The clang-format hg extension formats c++ at commit time. The js-format hg extension formats (and lints) js at commit time. These are all separate and can be individually turned on or off.

It might be better to make format-source smarter, however I'm unconvinced that it's worth pouring engineering effort into. I don't expect these kinds of tree-wide formatting changes to happen often. However it is certainly true we might want to standardize coding style for more languages.

This doesn't seem to answer the question I asked in comment 3. I want the "format on commit" behaviour. I also want reasonable rebase times. Is there currently a way to do that? (It sounds like the answer is "no")

If not, why shouldn't fixing this bug be a priority? It saves everyone time if code formatting isn't an issue that comes up at review time - in fact, I thought that was the primary reason we were doing all of this...

Comment 6

[Victor Porof [:vporof][:vp]]

To reiterate, the format-source, clang-format and js-format extensions are all separate and can be individually turned on or off.

Bug 1572072 has taught mach bootstrap to disable format-source unless users opt-in explicitly. This will speed up rebases and merges.
The js-format extension is unaffected and will continue to remain enabled after running bootstrap, which formats on commit.

Comment 7

[Connor Sheehan [:sheehan]]

We no longer use format-source in our workflows.