[PATCH v1 0/2] t/perf: bisect performance regressions

[PATCH v1 0/2] t/perf: bisect performance regressions

[Christian Couder]

This is a small patch series on top of cc/perf-aggregate-sort, which
is next, to add scripts that make it much easier to bisect performance
regressions.

For example if you ran perf test using the perf.conf config file that
contains a "with libpcre", then using:

$ ./aggregate.perl --subsection "with libpcre" --sort-by=regression v2.15.1 v2.16.2 v2.17.0 

you can get a line in the output like:

+8.2% p5302-pack-index.6 14.33(44.50+0.77) 15.50(43.83+0.94) v2.16.2 v2.17.0

and you can now use this output line to bisect where the regression
comes from using:

echo "+8.2% p5302-pack-index.6 14.33(44.50+0.77) 15.50(43.83+0.94) v2.16.2 v2.17.0" | ./bisect_regression --config perf.conf --subsection "with libpcre"

Caveats:

You must make sure that the times are consistent between runs and that
there is a significant time difference between the "good" runs and the
"bad" runs.

For example the following is not easily bisectable:

+100.0% p0000-perf-lib-sanity.2 0.01(0.01+0.01) 0.02(0.02+0.00) v2.15.1 v2.16.2

as the rtime went from 0.01 s to 0.02 s and we only have 0.01 s precision.

Christian Couder (2):
  perf/run: add --subsection option
  t/perf: add scripts to bisect performance regressions

 t/perf/bisect_regression | 73 ++++++++++++++++++++++++++++++++++++++++
 t/perf/bisect_run_script | 47 ++++++++++++++++++++++++++
 t/perf/run               | 56 ++++++++++++++++++++++++------
 3 files changed, 166 insertions(+), 10 deletions(-)
 create mode 100755 t/perf/bisect_regression
 create mode 100755 t/perf/bisect_run_script

-- 
2.17.0.rc1.36.g098d832c9.dirty

[PATCH v1 1/2] perf/run: add --subsection option

[Christian Couder]

This new option makes it possible to run perf tests as defined
in only one subsection of a config file.

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

diff --git a/t/perf/run b/t/perf/run
index 213da5d6b9..9aaa733c77 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -1,21 +1,34 @@
 #!/bin/sh
 
-case "$1" in
+die () {
+	echo >&2 "error: $*"
+	exit 1
+}
+
+while [ $# -gt 0 ]; do
+	arg="$1"
+	case "$arg" in
+	--)
+		break ;;
 	--help)
-		echo "usage: $0 [--config file] [other_git_tree...] [--] [test_scripts]"
-		exit 0
-		;;
+		echo "usage: $0 [--config file] [--subsection subsec] [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 () {
-	echo >&2 "error: $*"
-	exit 1
-}
+	--subsection)
+		shift
+		GIT_PERF_SUBSECTION="$1"
+		export GIT_PERF_SUBSECTION
+		shift ;;
+	--*)
+		die "unrecognised option: '$arg'" ;;
+	*)
+		break ;;
+	esac
+done
 
 run_one_dir () {
 	if test $# -eq 0; then
@@ -172,9 +185,32 @@ get_subsections "perf" >test-results/run_subsections.names
 
 if test $(wc -l <test-results/run_subsections.names) -eq 0
 then
+	if test -n "$GIT_PERF_SUBSECTION"
+	then
+		if test -n "$GIT_PERF_CONFIG_FILE"
+		then
+			die "no subsections are defined in config file '$GIT_PERF_CONFIG_FILE'"
+		else
+			die "subsection '$GIT_PERF_SUBSECTION' defined without a config file"
+		fi
+	fi
 	(
 		run_subsection "$@"
 	)
+elif test -n "$GIT_PERF_SUBSECTION"
+then
+	egrep "^$GIT_PERF_SUBSECTION\$" test-results/run_subsections.names >/dev/null ||
+		die "subsection '$GIT_PERF_SUBSECTION' not found in '$GIT_PERF_CONFIG_FILE'"
+
+	egrep "^$GIT_PERF_SUBSECTION\$" test-results/run_subsections.names | while read -r subsec
+	do
+		(
+			GIT_PERF_SUBSECTION="$subsec"
+			export GIT_PERF_SUBSECTION
+			echo "======== Run for subsection '$GIT_PERF_SUBSECTION' ========"
+			run_subsection "$@"
+		)
+	done
 else
 	while read -r subsec
 	do
-- 
2.17.0.rc1.36.g098d832c9.dirty

Re: [PATCH v1 1/2] perf/run: add --subsection option

[Eric Sunshine]

On Sun, Apr 8, 2018 at 5:35 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> This new option makes it possible to run perf tests as defined
> in only one subsection of a config file.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> diff --git a/t/perf/run b/t/perf/run
> @@ -1,21 +1,34 @@
> +while [ $# -gt 0 ]; do

I was going to ask if you meant to use 'test' here rather than '[',
however, I see that this script already has a mix of the two, so...

Likewise, scripts in Git usually omit the semicolon and place 'do' on
its own line, however, again I see that this script has a mix of the
two styles, so...

[PATCH v1 2/2] t/perf: add scripts to bisect performance regressions

[Christian Couder]

The new bisect_regression script can be used to automatically bisect
performance regressions. It will pass the new bisect_run_script to
`git bisect run`.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/perf/bisect_regression | 73 ++++++++++++++++++++++++++++++++++++++++
 t/perf/bisect_run_script | 47 ++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)
 create mode 100755 t/perf/bisect_regression
 create mode 100755 t/perf/bisect_run_script

diff --git a/t/perf/bisect_regression b/t/perf/bisect_regression
new file mode 100755
index 0000000000..a94d9955d0
--- /dev/null
+++ b/t/perf/bisect_regression
@@ -0,0 +1,73 @@
+#!/bin/sh
+
+# Read a line coming from `./aggregate.perl --sort-by regression ...`
+# and automatically bisect to find the commit responsible for the
+# performance regression.
+#
+# Lines from `./aggregate.perl --sort-by regression ...` look like:
+#
+# +100.0% p7821-grep-engines-fixed.1 0.04(0.10+0.03) 0.08(0.11+0.08) v2.14.3 v2.15.1
+# +33.3% p7820-grep-engines.1 0.03(0.08+0.02) 0.04(0.08+0.02) v2.14.3 v2.15.1
+#
+
+die () {
+	echo >&2 "error: $*"
+	exit 1
+}
+
+while [ $# -gt 0 ]; do
+	arg="$1"
+	case "$arg" in
+	--help)
+		echo "usage: $0 [--config file] [--subsection subsection]"
+		exit 0
+		;;
+	--config)
+		shift
+		GIT_PERF_CONFIG_FILE=$(cd "$(dirname "$1")"; pwd)/$(basename "$1")
+		export GIT_PERF_CONFIG_FILE
+		shift ;;
+	--subsection)
+		shift
+		GIT_PERF_SUBSECTION="$1"
+		export GIT_PERF_SUBSECTION
+		shift ;;
+	--*)
+		die "unrecognised option: '$arg'" ;;
+	*)
+		die "unknown argument '$arg'"
+		;;
+	esac
+done
+
+read -r regression subtest oldtime newtime oldrev newrev
+
+test_script=$(echo "$subtest" | sed -e 's/\(.*\)\.[0-9]*$/\1.sh/')
+test_number=$(echo "$subtest" | sed -e 's/.*\.\([0-9]*\)$/\1/')
+
+# oldtime and newtime are decimal number, not integers
+
+oldtime=$(echo "$oldtime" | sed -e 's/^\([0-9]\+\.[0-9]\+\).*$/\1/')
+newtime=$(echo "$newtime" | sed -e 's/^\([0-9]\+\.[0-9]\+\).*$/\1/')
+
+test $(echo "$newtime" "$oldtime" | awk '{ print ($1 > $2) }') = 1 ||
+	die "New time '$newtime' shoud be greater than old time '$oldtime'"
+
+tmpdir=$(mktemp -d -t bisect_regression_XXXXXX) || die "Failed to create temp directory"
+echo "$oldtime" >"$tmpdir/oldtime" || die "Failed to write to '$tmpdir/oldtime'"
+echo "$newtime" >"$tmpdir/newtime" || die "Failed to write to '$tmpdir/newtime'"
+
+# Bisecting must be performed from the top level directory (even with --no-checkout)
+(
+	toplevel_dir=$(git rev-parse --show-toplevel) || die "Failed to find top level directory"
+	cd "$toplevel_dir" || die "Failed to cd into top level directory '$toplevel_dir'"
+
+	git bisect start --no-checkout "$newrev" "$oldrev" || die "Failed to start bisecting"
+
+	git bisect run t/perf/bisect_run_script "$test_script" "$test_number" "$tmpdir"
+	res="$?"
+
+	git bisect reset
+
+	exit "$res"
+)
diff --git a/t/perf/bisect_run_script b/t/perf/bisect_run_script
new file mode 100755
index 0000000000..038255df4b
--- /dev/null
+++ b/t/perf/bisect_run_script
@@ -0,0 +1,47 @@
+#!/bin/sh
+
+script="$1"
+test_number="$2"
+info_dir="$3"
+
+# This aborts the bisection immediately
+die () {
+	echo >&2 "error: $*"
+	exit 255
+}
+
+bisect_head=$(git rev-parse --verify BISECT_HEAD) || die "Failed to find BISECT_HEAD ref"
+
+script_number=$(echo "$script" | sed -e "s/^p\([0-9]*\).*\$/\1/") || die "Failed to get script number for '$script'"
+
+oldtime=$(cat "$info_dir/oldtime") || die "Failed to access '$info_dir/oldtime'"
+newtime=$(cat "$info_dir/newtime") || die "Failed to access '$info_dir/newtime'"
+
+cd t/perf || die "Failed to cd into 't/perf'"
+
+result_file="$info_dir/perf_${script_number}_${bisect_head}_results.txt"
+
+GIT_PERF_DIRS_OR_REVS="$bisect_head"
+export GIT_PERF_DIRS_OR_REVS
+
+./run "$script" >"$result_file" 2>&1 || die "Failed to run perf test '$script'"
+
+rtime=$(sed -n "s/^$script_number\.$test_number:.*\([0-9]\+\.[0-9]\+\)(.*).*\$/\1/p" "$result_file")
+
+echo "newtime: $newtime"
+echo "rtime: $rtime"
+echo "oldtime: $oldtime"
+
+# Compare ($newtime - $rtime) with ($rtime - $oldtime)
+# Times are decimal number, not integers
+
+if test $(echo "$newtime" "$rtime" "$oldtime" | awk '{ print ($1 - $2 > $2 - $3) }') = 1
+then
+	# Current commit is considered "good/old"
+	echo "$rtime" >"$info_dir/oldtime"
+	exit 0
+else
+	# Current commit is considered "bad/new"
+	echo "$rtime" >"$info_dir/newtime"
+	exit 1
+fi
-- 
2.17.0.rc1.36.g098d832c9.dirty