Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY

From: Jeff King <peff@peff.net>
Date: Jul 19, 2021, 08:25 UTC
Message-ID: <YPU2+bG8Nge5FKWv@coredump.intra.peff.net>
In-Reply-To: <cb9d948646e8181e9c9c8dc35b37d48c692a92fb.1624024532.git.ps@pks.im>

On Fri, Jun 18, 2021 at 03:56:08PM +0200, Patrick Steinhardt wrote:

Show 10 quoted lines

> When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be
> written in that directory instead of the default directory located in
> "t/". While this works as expected for our normal tests, performance
> tests fail to locate and aggregate performance data because they don't
> know to handle TEST_OUTPUT_DIRECTORY correctly and always look at the
> default location.
> 
> Fix the issue by adding a `--results-dir` parameter to "aggregate.perl"
> which identifies the directory where results are and by making the "run"
> script awake of the TEST_OUTPUT_DIRECTORY variable.

OK, that makes sense. My first thought is that the aggregation script could simply use $TEST_OUTPUT_DIRECTORY itself, but:

  - we don't actually export that (and changing that is likely to have
    undesirable secondary effects)

  - people may run aggregate separately anyway (I know I do in order to
    produce nice tables without having to pointlessly re-run all tests)

So the new option makes sense. It is a little less convenient when running aggregate manually (you have to say "--results-dir" manually), but it's better than not being able to do it at all. :)

A few notes / questions below:

Show 10 quoted lines

> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 601d9f67dd..f5ed092ee5 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
>  MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git
>  export MODERN_GIT
>  
> -perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
> +perf_results_dir=$TEST_RESULTS_DIR

This line puzzled me a bit. Isn't $TEST_RESULTS_DIR already defined to be $TEST_OUTPUT_DIRECTORY/test-results? If the change here is just for clarity / readability that's OK by me. I just want to make sure I'm not missing something.

Show 11 quoted lines

> @@ -253,7 +253,10 @@ test_size () {
>  # and does it after running everything)
>  test_at_end_hook_ () {
>  	if test -z "$GIT_PERF_AGGREGATING_LATER"; then
> -		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl $(basename "$0") )
> +		(
> +			cd "$TEST_DIRECTORY"/perf &&
> +			./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0")
> +		)
>  	fi
>  }

OK, and we pass it unconditionally, which should work because it will always be set, even if you aren't overriding the default. Good.

Show 18 quoted lines

> diff --git a/t/perf/run b/t/perf/run
> index c7b86104e1..d19dec258a 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -188,10 +188,10 @@ run_subsection () {
>  
>  	if test -z "$GIT_PERF_SEND_TO_CODESPEED"
>  	then
> -		./aggregate.perl $codespeed_opt "$@"
> +		./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $codespeed_opt "$@"
>  	else
> -		json_res_file="test-results/$GIT_PERF_SUBSECTION/aggregate.json"
> -		./aggregate.perl --codespeed "$@" | tee "$json_res_file"
> +		json_res_file=""$TEST_RESULTS_DIR"/$GIT_PERF_SUBSECTION/aggregate.json"
> +		./aggregate.perl --results-dir="$TEST_RESULTS_DIR" --codespeed "$@" | tee "$json_res_file"
>  		send_data_url="$GIT_PERF_SEND_TO_CODESPEED/result/add/json/"
>  		curl -v --request POST --data-urlencode "json=$(cat "$json_res_file")" "$send_data_url"
>  	fi

In the earlier hunks from perf-lib.sh, we got $TEST_RESULTS_DIR by sourcing test-lib.sh. But in the "run" script, we don't. So who sets it? Looks like...

Show 12 quoted lines

> @@ -203,10 +203,17 @@ get_var_from_env_or_config "GIT_PERF_SEND_TO_CODESPEED" "perf" "sendToCodespeed"
>  cd "$(dirname $0)"
>  . ../../GIT-BUILD-OPTIONS
>  
> -mkdir -p test-results
> -get_subsections "perf" >test-results/run_subsections.names
> +if test -n "$TEST_OUTPUT_DIRECTORY"
> +then
> +	TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
> +else
> +	TEST_RESULTS_DIR=test-results
> +fi

...we now do. Makes sense. And we will get TEST_OUTPUT_DIRECTORY directly from GIT-BUILD-OPTIONS (or it won't be set at all if unspecified).

So the whole thing looks good to me, assuming there is nothing confusing about the one assignment I mentioned.

-Peff