Re: [WIP/PATCH 7/6] perf: add a performance test for core.fsmonitor

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 4, 2017, 07:46 UTC
Message-ID: <CACBZZX4XOaN8o89vetoU8NMqRH+BaqHGkxq77MpqzvAM40exEA@mail.gmail.com>
In-Reply-To: <xmqq1sr0wm0i.fsf@gitster.mtv.corp.google.com>

On Sun, Jun 4, 2017 at 3:59 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> This is WIP code for the reasons explained in the setup comments,
>> unfortunately the perf code doesn't easily allow you to run different
>> setup code for different versions you're testing. This test will stop
>> working if the fsmonitor is merged into the master branch.
>> ...
>> +
>> +     # Relies on core.fsmonitor not being merged into master. Needs
>> +     # better per-test ways to disable it if it gets merged.
>> +     git config core.fsmonitor true &&
>
> Will stop working and relies on not merged can be read but I cannot
> read "why" explained, and I cannot quite guess what the reason is.
>
> If the code to read the configuration is not there, setting this
> would not have any effect.  If the code is there, setting this would
> have effect (either talking fsmonitor helps or it hurts).
>
> And I do not think we'd ever see a version of Git that always relies
> on talking to fsmonitor, i.e. "git config core.fsmonitor false" is not
> a way to disable it, so...
>
> Puzzled.

Sorry about the unclear brevity.

What I'm referring to is not a limitation of git (we'll always be able to turn off core.fsmonitor), but a limitation of the perf framework. There's no way to run a test like this:

    ./run master next -- p*.sh

And have some information passed to the test to apply different runtime options to the test depending on master or next, or be to test master twice, once with the fsmonitor, once without, which this hypothetical feature would do:

    ./run master:"GIT_PERF_7519_NO_FSMONITOR=Y" master -- p*.sh

So right now the test works because there's no core.fsmonitor in master, so turning it on all the time only impacts avar/fsmonitor, not master.

I started trying to add this to the perf framework the other day but ran out of time, the option should also be passed down early enough to be intercepted by the GIT_PERF_MAKE_COMMAND, so you could do e.g.:

    GIT_PERF_MAKE_COMMAND='make CFLAGS="$F"' \
        ./run v2.13.0:"F=-O0" v2.13.0:"F=-O1" v2.13.0:"F=-O2"
v2.13.0:"F=-O3" -- p*.sh

To test the same revision with different compilation flags.

A change like this would break the ability to enact certain perf optimizations, right now we unpack the revision(s) you specify into <sha1-it-points-to>, and e.g. make use of the fact that that directory is already unpacked so we don't need to unpack it again.

If there was no way to pass a flag to specific revisions being tested, then perf could just optimize:

    ./run v2.12.0 v2.13.0 b06d364310 -- p*.sh

To skip the b06d364310 run entirely since it's the same as v2.13.0. I think breaking this minor assumption in the framework is fine, but it's worth noting that it's an assumption we couldn't make anymore.