[PATCH v1 0/4] Teach 'run' perf script to read config files

[PATCH v1 0/4] Teach 'run' perf script to read config files

[Christian Couder]

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 will make it easier to store, reuse,
share and compare parameters.

In the future it could also make 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.

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

Patch 1/4 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/4 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/4 and 4/4 use the get_var_from_env_or_config() function to
make it possible to set parameters used by the 'run' script.  

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

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 could 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 CFLAGS='-g -O0' USE_LIBPCRE=YesPlease
[perf "without libpcre"]
        makeOpts = DEVELOPER=1 CFLAGS='-g -O0'

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

But this can be added later, and the current series can already be
useful as is.

Links
~~~~~

This patch series is also available here:

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


Christian Couder (4):
  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()

 t/perf/perf-lib.sh |  3 ---
 t/perf/run         | 34 +++++++++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.13.2.647.g8b2efe2a0f

[PATCH v1 1/4] perf/run: add '--config' option to the 'run' script

[Christian Couder]

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

Let's make it easy to store some parameters in a config
file and to pass them to the run script.

The GIT_PERF_CONFIG_FILE variable will be set to the
argument of the '--config' option. This variable is not
used yet. It will be used in a following commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/perf/run | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/perf/run b/t/perf/run
index beb4acc0e4..1e7c2a59e4 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -2,9 +2,14 @@
 
 case "$1" in
 	--help)
-		echo "usage: $0 [other_git_tree...] [--] [test_scripts]"
+		echo "usage: $0 [--config file] [other_git_tree...] [--] [test_scripts]"
 		exit 0
 		;;
+	--config)
+		shift
+		GIT_PERF_CONFIG_FILE=$(cd "$(dirname "$1")"; pwd)/$(basename "$1")
+		export GIT_PERF_CONFIG_FILE
+		shift ;;
 esac
 
 die () {
-- 
2.13.2.647.g8b2efe2a0f

[PATCH v1 2/4] perf/run: add get_var_from_env_or_config()

[Christian Couder]

Add get_var_from_env_or_config() to easily set variables
from a config file if they are defined there and not already set.

This can also set them to a default value if one is provided.

As an example, use this function to set GIT_PERF_REPEAT_COUNT
from the perf.repeatCount config option or from the default
value.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/perf/perf-lib.sh |  3 ---
 t/perf/run         | 21 +++++++++++++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index b50211b259..2f88fc12a9 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -59,9 +59,6 @@ perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
 mkdir -p "$perf_results_dir"
 rm -f "$perf_results_dir"/$(basename "$0" .sh).subtests
 
-if test -z "$GIT_PERF_REPEAT_COUNT"; then
-	GIT_PERF_REPEAT_COUNT=3
-fi
 die_if_build_dir_not_repo () {
 	if ! ( cd "$TEST_DIRECTORY/.." &&
 		    git rev-parse --build-dir >/dev/null 2>&1 ); then
diff --git a/t/perf/run b/t/perf/run
index 1e7c2a59e4..41580ac6df 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -34,6 +34,7 @@ unpack_git_rev () {
 	(cd "$(git rev-parse --show-cdup)" && git archive --format=tar $rev) |
 	(cd build/$rev && tar x)
 }
+
 build_git_rev () {
 	rev=$1
 	for config in config.mak config.mak.autogen config.status
@@ -92,6 +93,26 @@ run_dirs () {
 	done
 }
 
+get_var_from_env_or_config () {
+	env_var="$1"
+	conf_var="$2"
+	# $3 can be set to a default value
+
+	# Do nothing if the env variable is already set
+	eval "test -z \"\${$env_var+x}\"" || return
+
+	# Check if the variable is in the config file
+	test -n "$GIT_PERF_CONFIG_FILE" &&
+	conf_value=$(git config -f "$GIT_PERF_CONFIG_FILE" "$conf_var") &&
+	eval "$env_var=\"$conf_value\"" || {
+		test -n "${3+x}" &&
+		eval "$env_var=\"$3\""
+	}
+}
+
+get_var_from_env_or_config "GIT_PERF_REPEAT_COUNT" "perf.repeatCount" 3
+export GIT_PERF_REPEAT_COUNT
+
 GIT_PERF_AGGREGATING_LATER=t
 export GIT_PERF_AGGREGATING_LATER
 
-- 
2.13.2.647.g8b2efe2a0f

[PATCH v1 3/4] perf/run: add GIT_PERF_DIRS_OR_REVS

[Christian Couder]

This environment variable can be set to some revisions or
directories whose Git versions should be tested, in addition
to the revisions or directories passed as arguments to the
'run' script.

This enables a "perf.dirsOrRevs" configuration variable to
be used to set revisions or directories whose Git versions
should be tested.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/perf/run | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/perf/run b/t/perf/run
index 41580ac6df..ad442fe64a 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -113,6 +113,9 @@ get_var_from_env_or_config () {
 get_var_from_env_or_config "GIT_PERF_REPEAT_COUNT" "perf.repeatCount" 3
 export GIT_PERF_REPEAT_COUNT
 
+get_var_from_env_or_config "GIT_PERF_DIRS_OR_REVS" "perf.dirsOrRevs"
+set -- $GIT_PERF_DIRS_OR_REVS "$@"
+
 GIT_PERF_AGGREGATING_LATER=t
 export GIT_PERF_AGGREGATING_LATER
 
-- 
2.13.2.647.g8b2efe2a0f

[PATCH v1 4/4] perf/run: add calls to get_var_from_env_or_config()

[Christian Couder]

These calls make it possible to have the make command or the
make options in a config file, instead of in environment
variables.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/perf/run | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/perf/run b/t/perf/run
index ad442fe64a..6bd15e7017 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -116,6 +116,9 @@ export GIT_PERF_REPEAT_COUNT
 get_var_from_env_or_config "GIT_PERF_DIRS_OR_REVS" "perf.dirsOrRevs"
 set -- $GIT_PERF_DIRS_OR_REVS "$@"
 
+get_var_from_env_or_config "GIT_PERF_MAKE_COMMAND" "perf.makeCommand"
+get_var_from_env_or_config "GIT_PERF_MAKE_OPTS" "perf.makeOpts"
+
 GIT_PERF_AGGREGATING_LATER=t
 export GIT_PERF_AGGREGATING_LATER
 
-- 
2.13.2.647.g8b2efe2a0f

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

[Jeff King]

On Thu, Jul 13, 2017 at 08:50:46AM +0200, Christian Couder wrote:

> 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 will make it easier to store, reuse,
> share and compare parameters.

Because perf-lib is built on test-lib, it already reads
GIT-BUILD-OPTIONS. And the Makefile copies several perf-related values
into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you
can already do:

  echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak
  echo 'GIT_PERF_MAKE_OPTS = CFLAGS="-O2" DEVELOPER=1' >>config.mak
  make
  cd t/perf
  ./run <versions-and-scripts>

I suspect there are still a lot of things that could be made easier with
a config file, so I'm not against the concept. Your example here:

> [perf "with libpcre"]
>         makeOpts = DEVELOPER=1 CFLAGS='-g -O0' USE_LIBPCRE=YesPlease
> [perf "without libpcre"]
>         makeOpts = DEVELOPER=1 CFLAGS='-g -O0'

is a lot more compelling. But right now the perf suite is not useful at
all for comparing two builds of the same tree. For that, I think it
would be more useful if we could define a tuple of parameters for a run.
One of which could be the tree we're testing. Build opts are another.
Tested repository is another. And then we'd fill in a table of results
and let you slice up the table by any column (e.g., compare times for
runs against a single tree but with differing build options).

So then I think your config file primarily becomes about defining the
properties of each run. I'm not sure if it would look like what you're
starting on here or not.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Because perf-lib is built on test-lib, it already reads
> GIT-BUILD-OPTIONS. And the Makefile copies several perf-related values
> into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you
> can already do:
> ...
> But right now the perf suite is not useful at
> all for comparing two builds of the same tree.
> For that, I think it
> would be more useful if we could define a tuple of parameters for a run.
> One of which could be the tree we're testing. Build opts are another.
> Tested repository is another. And then we'd fill in a table of results
> and let you slice up the table by any column (e.g., compare times for
> runs against a single tree but with differing build options).

Yeah, I think we saw this discussed in not-so-distant past, for
which we want a good solution, and it might be the case that such a
solution can be made easier to use with a separate configuration
file (which this topic may or may not be used as a building block).

> So then I think your config file primarily becomes about defining the
> properties of each run. I'm not sure if it would look like what you're
> starting on here or not.

Yeah, I suspect that the final shape that defines the matrix might
have to become quite a bit different.

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

[Jeff King]

On Thu, Jul 13, 2017 at 11:29:10AM -0700, Junio C Hamano wrote:

> > So then I think your config file primarily becomes about defining the
> > properties of each run. I'm not sure if it would look like what you're
> > starting on here or not.
> 
> Yeah, I suspect that the final shape that defines the matrix might
> have to become quite a bit different.

I think it would help if the perf code was split better into three
distinct bits:

  1. A data-store capable of storing the run tuples along with their
     outcomes for each test.

  2. A "run" front-end that runs various profiles (based on config,
     command-line options, etc) and writes the results to the data
     store.

  3. A flexible viewer which can slice and dice the contents of the data
     store according to different parameters.

We're almost there now. The "run" script actually does store results,
and you can view them via "aggregate.pl" without actually re-running the
tests. But the data store only indexes on one property: the tree that
was tested (and all of the other properties are ignored totally; you can
get some quite confusing results if you do a "./run" using say git.git
as your test repo, and then a followup with "linux.git").

I have to imagine that somebody else has written such a system already
that we could reuse.  I don't know of one off-hand, but this is also not
an area where I've spent a lot of time.

We're sort of drifting off topic from Christian's patches here. But if
we did have a third-party system, I suspect the interesting work would
be setting up profiles for the "run" tool to kick off. And we might be
stuck in such a case using whatever format the tool prefers. So having a
sense of what the final solution looks like might help us know whether
it makes sense to introduce a custom config format here.

-Peff

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

[Christian Couder]

On Thu, Jul 13, 2017 at 6:58 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jul 13, 2017 at 08:50:46AM +0200, Christian Couder wrote:
>
>> 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 will make it easier to store, reuse,
>> share and compare parameters.
>
> Because perf-lib is built on test-lib, it already reads
> GIT-BUILD-OPTIONS.

Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe
this is not necessary.
Also are the variables in GIT-BUILD-OPTIONS exported already?

> And the Makefile copies several perf-related values
> into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you
> can already do:
>
>   echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak
>   echo 'GIT_PERF_MAKE_OPTS = CFLAGS="-O2" DEVELOPER=1' >>config.mak
>   make

The "make" here might not even be needed as in the 'run' script
"config.mak" is copied into the "build/$rev" directory where "make" is
run to build the $rev version.

>   cd t/perf
>   ./run <versions-and-scripts>
>
> I suspect there are still a lot of things that could be made easier with
> a config file, so I'm not against the concept. Your example here:
>
>> [perf "with libpcre"]
>>         makeOpts = DEVELOPER=1 CFLAGS='-g -O0' USE_LIBPCRE=YesPlease
>> [perf "without libpcre"]
>>         makeOpts = DEVELOPER=1 CFLAGS='-g -O0'
>
> is a lot more compelling. But right now the perf suite is not useful at
> all for comparing two builds of the same tree. For that, I think it
> would be more useful if we could define a tuple of parameters for a run.
> One of which could be the tree we're testing. Build opts are another.
> Tested repository is another. And then we'd fill in a table of results
> and let you slice up the table by any column (e.g., compare times for
> runs against a single tree but with differing build options).

Yeah, improving the output part is another thing that I have discussed
with AEvar and that I have planned to work on.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... But if
> we did have a third-party system, I suspect the interesting work would
> be setting up profiles for the "run" tool to kick off. And we might be
> stuck in such a case using whatever format the tool prefers. So having a
> sense of what the final solution looks like might help us know whether
> it makes sense to introduce a custom config format here.

Agreed.

Thanks.

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

[Christian Couder]

On Thu, Jul 13, 2017 at 8:40 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jul 13, 2017 at 11:29:10AM -0700, Junio C Hamano wrote:
>
>> > So then I think your config file primarily becomes about defining the
>> > properties of each run. I'm not sure if it would look like what you're
>> > starting on here or not.
>>
>> Yeah, I suspect that the final shape that defines the matrix might
>> have to become quite a bit different.
>
> I think it would help if the perf code was split better into three
> distinct bits:
>
>   1. A data-store capable of storing the run tuples along with their
>      outcomes for each test.
>
>   2. A "run" front-end that runs various profiles (based on config,
>      command-line options, etc) and writes the results to the data
>      store.
>
>   3. A flexible viewer which can slice and dice the contents of the data
>      store according to different parameters.
>
> We're almost there now. The "run" script actually does store results,
> and you can view them via "aggregate.pl" without actually re-running the
> tests. But the data store only indexes on one property: the tree that
> was tested (and all of the other properties are ignored totally; you can
> get some quite confusing results if you do a "./run" using say git.git
> as your test repo, and then a followup with "linux.git").

Yeah I agree, but if possible I'd like to avoid working on the three
different parts at the same time.

I haven't thought much about how to improve the data store yet.
I may have to look at that soon though.

> I have to imagine that somebody else has written such a system already
> that we could reuse.  I don't know of one off-hand, but this is also not
> an area where I've spent a lot of time.

Actually about the viewer AEvar suggested having something like
speed.python.org and speed.pypy.org which seem to be made using
https://github.com/tobami/codespeed

So unless something else is suggested, I plan to make it possible to
import the results of the perf tests into codespeed, but I haven't
looked at that much yet.

> We're sort of drifting off topic from Christian's patches here. But if
> we did have a third-party system, I suspect the interesting work would
> be setting up profiles for the "run" tool to kick off. And we might be
> stuck in such a case using whatever format the tool prefers. So having a
> sense of what the final solution looks like might help us know whether
> it makes sense to introduce a custom config format here.

I don't think we should completely switch to a third-party system for
everything.
Though it would simplify my work if we decide to do that.

I think people might want different viewers, so we should just make
sure that we can easily massage the results from the run script, so
that it will be easy to provide them as input to many different
viewers.

So we are pretty free to decide how we specify which tests should be
performed on which revision, and I think a config file is the best
way.

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

[Jeff King]

On Thu, Jul 13, 2017 at 08:57:01PM +0200, Christian Couder wrote:

> >> We want to make it possible to store the parameters to the 'run'
> >> script in a config file. This will make it easier to store, reuse,
> >> share and compare parameters.
> >
> > Because perf-lib is built on test-lib, it already reads
> > GIT-BUILD-OPTIONS.
> 
> Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe
> this is not necessary.

Ah, right. The one that comes via perf-lib gets the variables into the
test scripts themselves. But anything "run" would need itself would come
from the source it does itself. And that's where GIT_PERF_MAKE_OPTS has
an effect.

> Also are the variables in GIT-BUILD-OPTIONS exported already?

No, I don't think so. But because both "run" and the scripts themselves
source them, they're available more or less everywhere, except for
sub-processes inside the scripts.

> > And the Makefile copies several perf-related values
> > into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you
> > can already do:
> >
> >   echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak
> >   echo 'GIT_PERF_MAKE_OPTS = CFLAGS="-O2" DEVELOPER=1' >>config.mak
> >   make
> 
> The "make" here might not even be needed as in the 'run' script
> "config.mak" is copied into the "build/$rev" directory where "make" is
> run to build the $rev version.

You need it to bake the config into GIT-BUILD-OPTIONS, which is the only
thing that gets read by "run" and the perf scripts. If you are
just setting MAKE_OPTS to things that your config.mak already sets, then
yes, you can skip that one.

-Peff

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

[Christian Couder]

On Thu, Jul 13, 2017 at 10:55 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jul 13, 2017 at 08:57:01PM +0200, Christian Couder wrote:
>
>> >> We want to make it possible to store the parameters to the 'run'
>> >> script in a config file. This will make it easier to store, reuse,
>> >> share and compare parameters.
>> >
>> > Because perf-lib is built on test-lib, it already reads
>> > GIT-BUILD-OPTIONS.
>>
>> Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe
>> this is not necessary.
>
> Ah, right. The one that comes via perf-lib gets the variables into the
> test scripts themselves. But anything "run" would need itself would come
> from the source it does itself. And that's where GIT_PERF_MAKE_OPTS has
> an effect.
>
>> Also are the variables in GIT-BUILD-OPTIONS exported already?
>
> No, I don't think so. But because both "run" and the scripts themselves
> source them, they're available more or less everywhere, except for
> sub-processes inside the scripts.

Ok, I see.

>> > And the Makefile copies several perf-related values
>> > into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you
>> > can already do:
>> >
>> >   echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak
>> >   echo 'GIT_PERF_MAKE_OPTS = CFLAGS="-O2" DEVELOPER=1' >>config.mak
>> >   make
>>
>> The "make" here might not even be needed as in the 'run' script
>> "config.mak" is copied into the "build/$rev" directory where "make" is
>> run to build the $rev version.
>
> You need it to bake the config into GIT-BUILD-OPTIONS, which is the only
> thing that gets read by "run" and the perf scripts. If you are
> just setting MAKE_OPTS to things that your config.mak already sets, then
> yes, you can skip that one.

Thanks for the explanations.

The whole thing seems really complex to me though. And this makes me
think that people might want to specify different GIT-BUILD-OPTIONS
and config.mak files to be used when running perf tests, so that the
results from perf tests can easily be reproduced later even if they
have changed their build options in their development Git repo in the
meantime.

So perhaps the config file should make it possible to specify a
directory where all the build files (GIT-BUILD-OPTIONS, config.mak,
config.mak.autogen and config.status) that should be used should be
taken. And then it could also let people change some variables to
override what is in those files which is needed to run perf tests with
different parameters.

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

[Jeff King]

On Fri, Jul 14, 2017 at 08:27:53AM +0200, Christian Couder wrote:

> The whole thing seems really complex to me though. And this makes me
> think that people might want to specify different GIT-BUILD-OPTIONS
> and config.mak files to be used when running perf tests, so that the
> results from perf tests can easily be reproduced later even if they
> have changed their build options in their development Git repo in the
> meantime.

I agree with the complexity. The general idea is that your currently
built HEAD is a snapshot in time of options. But that doesn't have to be
so, and laying out the options in a config file does seem like an
improvement.

There is another implicit dependency, though: the set of (and exact
content of) the tests depends on your HEAD, too. So if I do:

  git checkout v2.5.0
  cd t/perf
  ./run v2.0.0 v2.1.0

I might get different results if I replace "v2.5.0" in the first command
with some other version, because the content of the tests will be
different. I'm not sure how to account for that in storing results. Most
of the time the version of the tests you ran is not going to be
interesting. But it can be a source of confusing discrepancies if a test
subtly changed between two runs. It probably happens infrequently enough
that it's not worth worrying about.

> So perhaps the config file should make it possible to specify a
> directory where all the build files (GIT-BUILD-OPTIONS, config.mak,
> config.mak.autogen and config.status) that should be used should be
> taken. And then it could also let people change some variables to
> override what is in those files which is needed to run perf tests with
> different parameters.

That sounds reasonable. I think you could ditch GIT-BUILD-OPTIONS
entirely. It's only needed to pull in GIT_PERF variables that would be
better served by being in the config in the first place.

-Peff

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

[Christian Couder]

On Fri, Jul 14, 2017 at 8:27 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Thu, Jul 13, 2017 at 10:55 PM, Jeff King <peff@peff.net> wrote:
>> On Thu, Jul 13, 2017 at 08:57:01PM +0200, Christian Couder wrote:
>>
>>> >> We want to make it possible to store the parameters to the 'run'
>>> >> script in a config file. This will make it easier to store, reuse,
>>> >> share and compare parameters.
>>> >
>>> > Because perf-lib is built on test-lib, it already reads
>>> > GIT-BUILD-OPTIONS.
>>>
>>> Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe
>>> this is not necessary.
>>
>> Ah, right. The one that comes via perf-lib gets the variables into the
>> test scripts themselves. But anything "run" would need itself would come
>> from the source it does itself. And that's where GIT_PERF_MAKE_OPTS has
>> an effect.
>>
>>> Also are the variables in GIT-BUILD-OPTIONS exported already?
>>
>> No, I don't think so. But because both "run" and the scripts themselves
>> source them, they're available more or less everywhere, except for
>> sub-processes inside the scripts.
>
> Ok, I see.

Actually after taking another look at that, it looks like the following happens:

1) the run script sources the original GIT-BUILD-OPTIONS file from
../.. relative to its location
2) a git version is built in "build/$rev" using GIT_PERF_MAKE_OPTS
which generates a new GIT-BUILD-OPTIONS file in "build/$rev/"
3) when the actual perf scripts are run they source the original
GIT-BUILD-OPTIONS file (through perf-lib.sh which sources test-lib.sh)

I wonder how useful 1) is, as the variables sourced from original
GIT-BUILD-OPTIONS are not used inside the "run" script and not
available to its child processes as they are not exported.
Is it just so that if people add GIT_PERF_* variables to their
config.mak before building they can then have those variables used by
the run script?

I also wonder if it would be better at step 3) to source the
GIT-BUILD-OPTIONS file generated at step 2) instead of the original
one, because they can be different as the options in
$GIT_PERF_MAKE_OPTS will be baked into the new GIT-BUILD-OPTIONS file.
(Of course if $GIT_PERF_MAKE_OPTS was added to config.mak before
building, then they will be in the original one too. But
$GIT_PERF_MAKE_OPTS should work without that.)

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

[Jeff King]

On Wed, Jul 26, 2017 at 05:58:09PM +0200, Christian Couder wrote:

> Actually after taking another look at that, it looks like the following happens:
> 
> 1) the run script sources the original GIT-BUILD-OPTIONS file from
> ../.. relative to its location
> 2) a git version is built in "build/$rev" using GIT_PERF_MAKE_OPTS
> which generates a new GIT-BUILD-OPTIONS file in "build/$rev/"
> 3) when the actual perf scripts are run they source the original
> GIT-BUILD-OPTIONS file (through perf-lib.sh which sources test-lib.sh)

Right, the perf scripts are run in the context of the "outer"
repository, and get their options from that one. I think that's
intentional, and does the right thing for GIT_PERF_* options. It's
possibly confusing if the tests really do want to know about the build
options for a particular test-build (like asking if it was built with
NO_PERL, for example).

I think in practice it works out OK, because we tend to do test-builds
that are similar to what's in the outer repo (because we copy
config.mak, and don't tend to add a lot of command-line options). But if
you put exotic options into your GIT_PERF_MAKE_OPTS, they won't be
reflected in the config that the test scripts see.

> I wonder how useful 1) is, as the variables sourced from original
> GIT-BUILD-OPTIONS are not used inside the "run" script and not
> available to its child processes as they are not exported.
> Is it just so that if people add GIT_PERF_* variables to their
> config.mak before building they can then have those variables used by
> the run script?

Exactly. I put GIT_PERF_MAKE_OPTS in my config.mak, and they end up
respected for each run without me having to specify them manually.

> I also wonder if it would be better at step 3) to source the
> GIT-BUILD-OPTIONS file generated at step 2) instead of the original
> one, because they can be different as the options in
> $GIT_PERF_MAKE_OPTS will be baked into the new GIT-BUILD-OPTIONS file.
> (Of course if $GIT_PERF_MAKE_OPTS was added to config.mak before
> building, then they will be in the original one too. But
> $GIT_PERF_MAKE_OPTS should work without that.)

I think that would make some cases work (build options for the tested
build), but I fear that it would break others (perf variables that
probably should be coming from the "outer" layer). Remember that test
builds may not be current versions and may not be forwarding those
variables via GIT-BUILD-OPTIONS. I think it's important that the bundle
of t/perf scripts act as a single unit that is driven primarily by the
currently checked-out version (and it's up to those scripts to handle
inconsistencies in old versions; see the $MODERN_GIT stuff I added a few
months ago.

Right now I don't think it has been a big problem, because the build
config tends to be the same.  But if we introduce more "properties" that
the user can tweak for a certain test run, this distinction is probably
going to cause more bugs. I'd almost say that the perf scripts should be
a project outside of git.git entirely, to eliminate confusion.

-Peff