Page MenuHomePhabricator

fix: reduce number of tool executions
ClosedPublic

Authored by hooper on Aug 11 2021, 10:16 PM.

Details

Summary

By grouping together (path, ctx) pairs according to the inputs they would
provide to fixer tools, we can deduplicate executions of fixer tools to
significantly reduce the amount of time spent running slow tools.

This change does not handle clean files in the working copy, which could still
be deduplicated against the files in the checked out commit. It's a little
harder to do that because the filerev is not available in the workingfilectx
(and it doesn't exist for added files).

Anecdotally, this change makes some real uses cases at Google 10x faster. I
think we were originally hesitant to do this because the benefits weren't
obvious, and implementing it efficiently is kind of tricky. If we simply
memoized the formatter execution function, we would be keeping tons of file
content in memory.

Also included is a regression test for a corner case that I broke with my first
attempt at optimizing this code.

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

hooper created this revision.Aug 11 2021, 10:16 PM
Alphare added inline comments.
hgext/fix.py
399–400

Maybe update the docstring to reflect the new grouping?

551–553

very minor nit: I usually find it clearer to prefix unused variables with _

hooper marked 2 inline comments as done.Sep 2 2021, 6:07 PM
hooper edited the summary of this revision. (Show Details)
hooper updated this revision to Diff 30179.
hooper updated this revision to Diff 30330.Mon, Sep 20, 4:29 PM

It looks like this somehow makes the Python 2 output flaky : https://foss.heptapod.net/octobus/mercurial-devel/-/jobs/247264

We're removing Python 2 support next release, you can choose to wait until then if you don't want to bother finding out why that happens. If so, ping me again on this change if I forget about it. :)

Alphare requested changes to this revision.Tue, Sep 21, 8:12 AM
This revision now requires changes to proceed.Tue, Sep 21, 8:12 AM
hooper updated this revision to Diff 30710.Mon, Oct 11, 10:03 PM

It looks like this somehow makes the Python 2 output flaky : https://foss.heptapod.net/octobus/mercurial-devel/-/jobs/247264

It was a dict iteration order thing. Fixed it by sorting on "(min(dstrevs), path, dstrevs)" instead of "min(dstrevs)". The important part is "path", and "dstrevs" should never be needed as a tie breaker.

pulkit accepted this revision.Tue, Oct 12, 9:33 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.