[PATCH v2 0/9] Teach 'run' perf script to read config files

[PATCH v2 0/9] Teach 'run' perf script to read config files

[Christian Couder]

(It looks like smtp.gmail.com isn't working anymore for me, so I am
trying to send this using Gmail for the cover letter and Submitgit for
the patches.)

Goal
~~~~

Using many long environment variables to give parameters to the 'run'
script is error prone and tiring.

We want to make it possible to store the parameters to the 'run'
script in a config file. This makes it easier to store, reuse,
share and compare parameters.

It also makes it easy to run series of tests.

Design
~~~~~~

We use the same config format as the ".git/config" file as Git users
and developers are familiar with this nice format and have great tools
to change, query and manage it.

We use values from the config file to set the environment variables
that are used by the scripts if they are not already set.

We want to make it possible to run series of tests by passing only a
config file to the 'run' script.

For example a config file like the following can be used to run perf
tests with Git compiled both with and without libpcre:

[perf]
        dirsOrRevs = v2.12.0 v2.13.0
        repeatCount = 10
[perf "with libpcre"]
        makeOpts = "DEVELOPER=1 USE_LIBPCRE=YesPlease"
[perf "without libpcre"]
        makeOpts = "DEVELOPER=1"

This makes it easy to see what changes between the different runs.

It's also possible (though maybe not so useful) to just separate tests
from different versions like this:

[perf]
        repeatCount = 2
        makeOpts = "DEVELOPER=1 USE_LIBPCRE=YesPlease"

[perf "with v2.12.0 and v2.13.1"]
        dirsOrRevs = v2.12.0 v2.13.1
[perf "with v2.11.0 and v2.12.1"]
        dirsOrRevs = v2.11.0 v2.12.1

Highlevel view of the patches in the series
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  - Patches 1/9 to 4/9 were already in v1 and haven't changed.

Patch 1/9 teaches the '--config <configfile>' option to the 'run'
script, but <configfile> is just put into the GIT_PERF_CONFIG_FILE
variable which is not used yet.

Patch 2/9 add the get_var_from_env_or_config() function to read config
options from the <configfile> and set values to some variables from
these config options or from default values.

Patch 3/9 and 4/9 use the get_var_from_env_or_config() function to
make it possible to set parameters used by the 'run' script.

  - Patches 5/9 to 9/9 are new.

Patch 5/9 introduce a function to deal with subsections in the config
file.

Patch 6/9 improves the get_var_from_env_or_config() function so that
it can handle subsections.

Patch 7/9 adds the run_subsection() function to run the tests for a
subsection.

Patch 8/9 improves the output when building a rev.

Patch 9/9 stores subsection results into subdirectories of test-results
so that results from previous subsections is not overwritten.

Future work
~~~~~~~~~~~

In the future I may work on the following:

  - improving aggregate.perl so that it can aggregates the results in
    different ways and formats, especially so that the results can be
    used by Codespeed (https://github.com/tobami/codespeed)
  - making it possible to configure more things in the config file
  - improving how GIT-BUILD-OPTIONS is handled

Though I think the series does not need the above improvements to be
already valuable.

Links
~~~~~

This patch series is also available here:

  https://github.com/chriscool/git/commits/perf-conf

Links to the previous version of this series are:

v1:
  https://github.com/chriscool/git/commits/perf-conf5
  https://public-inbox.org/git/20170713065050.19215-1-chriscool@tuxfamily.org/

Christian Couder (9):
  perf/run: add '--config' option to the 'run' script
  perf/run: add get_var_from_env_or_config()
  perf/run: add GIT_PERF_DIRS_OR_REVS
  perf/run: add calls to get_var_from_env_or_config()
  perf/run: add get_subsections()
  perf/run: update get_var_from_env_or_config() for subsections
  perf/run: add run_subsection()
  perf/run: show name of rev being built
  perf: store subsection results in "test-results/$GIT_PERF_SUBSECTION/"

 t/perf/aggregate.perl | 11 +++++--
 t/perf/perf-lib.sh    |  4 +--
 t/perf/run            | 89 +++++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 89 insertions(+), 15 deletions(-)

-- 
2.14.1.767.g2dbbf9317b

Re: [PATCH v2 0/9] Teach 'run' perf script to read config files

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> (It looks like smtp.gmail.com isn't working anymore for me, so I am
> trying to send this using Gmail for the cover letter and Submitgit for
> the patches.)

SubmitGit may want to learn the "change the timestamps of the
individual patches by 1 second" trick from "git send-email" to help
threading (you can view inbox/comp.version-control.git/ group over
nntp and tell your newsreader to sort-by-date).

> Highlevel view of the patches in the series
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   - Patches 1/9 to 4/9 were already in v1 and haven't changed.

It isn't quite clear what _did_ change in the series and what
lessons were learned form the previous round's discussion here.  The
sample configuration in the description above (snipped) seems to
have been extended and it shows that one of the use cases of the
feature is to allow comparing runs against two versions, which
looked more or less sensible way to express it.

> Christian Couder (9):
>   perf/run: add '--config' option to the 'run' script
>   perf/run: add get_var_from_env_or_config()
>   perf/run: add GIT_PERF_DIRS_OR_REVS
>   perf/run: add calls to get_var_from_env_or_config()
>   perf/run: add get_subsections()
>   perf/run: update get_var_from_env_or_config() for subsections
>   perf/run: add run_subsection()
>   perf/run: show name of rev being built
>   perf: store subsection results in "test-results/$GIT_PERF_SUBSECTION/"
>
>  t/perf/aggregate.perl | 11 +++++--
>  t/perf/perf-lib.sh    |  4 +--
>  t/perf/run            | 89 +++++++++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 89 insertions(+), 15 deletions(-)

Thanks.  Let me see how well it works ;-)

Re: [PATCH v2 0/9] Teach 'run' perf script to read config files

[Christian Couder]

On Sun, Sep 24, 2017 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> (It looks like smtp.gmail.com isn't working anymore for me, so I am
>> trying to send this using Gmail for the cover letter and Submitgit for
>> the patches.)
>
> SubmitGit may want to learn the "change the timestamps of the
> individual patches by 1 second" trick from "git send-email" to help
> threading (you can view inbox/comp.version-control.git/ group over
> nntp and tell your newsreader to sort-by-date).

Roberto is now in CC. I will let him answer about that.

>> Highlevel view of the patches in the series
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>   - Patches 1/9 to 4/9 were already in v1 and haven't changed.
>
> It isn't quite clear what _did_ change in the series and what
> lessons were learned form the previous round's discussion here.

In the previous round of discussion, I think the reviewers (you and
Peff) basically agreed that it was worth improving the test framework
to make it possible to easily run many tests, though reviewers were
not sure if what I had planned to do would in the end be a good
solution.

(See https://public-inbox.org/git/20170713065050.19215-1-chriscool@tuxfamily.org/)

So what did change is that I implemented what was in the "Future work"
section in v1.

> The
> sample configuration in the description above (snipped) seems to
> have been extended and it shows that one of the use cases of the
> feature is to allow comparing runs against two versions, which
> looked more or less sensible way to express it.

[...]

> Thanks.  Let me see how well it works ;-)

Thanks for testing ;-)

Re: [PATCH v2 0/9] Teach 'run' perf script to read config files

[Roberto Tyley]

On 26 September 2017 at 16:40, Christian Couder
<christian.couder@gmail.com> wrote:
> On Sun, Sep 24, 2017 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> (It looks like smtp.gmail.com isn't working anymore for me, so I am
>>> trying to send this using Gmail for the cover letter and Submitgit for
>>> the patches.)
>>
>> SubmitGit may want to learn the "change the timestamps of the
>> individual patches by 1 second" trick from "git send-email" to help
>> threading (you can view inbox/comp.version-control.git/ group over
>> nntp and tell your newsreader to sort-by-date).
>
> Roberto is now in CC. I will let him answer about that.

I had a quick look at git-send-email.perl, I see the trick is the `time++` one
introduced with https://github.com/git/git/commit/a5370b16 - seems reasonable!

SubmitGit makes all emails in-reply-to the initial email, which I
think is correct behaviour,
but I can see that offsetting the times would probably give a more
reliable sorting in
a newsreader. Unfortunately the documentation for AWS Simple Email Service (SES)
says:

  "Note: Amazon SES overrides any Date header you provide with the
time that Amazon
  SES accepts the message."

http://docs.aws.amazon.com/ses/latest/DeveloperGuide/header-fields.html

...so the only way SubmitGit can offset the times is to literally
delay the sending of the emails,
which is a bit unfortunate for patchbombs more than a few dozen commits long!

I'll take a further look at this when I get a bit more free time.

Roberto

Re: [PATCH v2 0/9] Teach 'run' perf script to read config files

[Stefan Beller]

>   "Note: Amazon SES overrides any Date header you provide with the
> time that Amazon
>   SES accepts the message."
>
> http://docs.aws.amazon.com/ses/latest/DeveloperGuide/header-fields.html
>
> ...so the only way SubmitGit can offset the times is to literally
> delay the sending of the emails,
> which is a bit unfortunate for patchbombs more than a few dozen commits long!

How many series of more than "a few dozen" patches were sent to the list lately?
I'd argue this is a non issue for the typical use case of submitGit.

> I'll take a further look at this when I get a bit more free time.

Thanks!
Stefan

Re: [PATCH v2 0/9] Teach 'run' perf script to read config files

[Junio C Hamano]

Roberto Tyley <roberto.tyley@gmail.com> writes:

> I had a quick look at git-send-email.perl, I see the trick is the `time++` one
> introduced with https://github.com/git/git/commit/a5370b16 - seems reasonable!
>
> SubmitGit makes all emails in-reply-to the initial email, which I
> think is correct behaviour,
> but I can see that offsetting the times would probably give a more
> reliable sorting in
> a newsreader. ...
> ...so the only way SubmitGit can offset the times is to literally
> delay the sending of the emails,
> which is a bit unfortunate for patchbombs more than a few dozen commits long!

As this matters mostly for a series that is longer than several
patches, it indeed is unfortunate.  If SubmitGit needs to send and
sleep for a dozen messages, does it mean the end user has to wait 12
(or is it 11? ;-)) seconds?  If the human is the only thing that
needs waiting, it may be OK (after all, it all happens in the web
browser and the human can switch to other tasks after clicking
"submit"), but that may not be acceptable if this artificial delay
can cause a timeout in a moving part among many.

Thanks for looking into this.