threads / patch / 48125

patch, 2 partsperf/aggregate: sort result by regression

Subject: [PATCH v1 0/2] perf/aggregate: sort result by regression

## tl;dr

6 messages between Mar 23, 2018 and Mar 24, 2018. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Christian Couder· Mar 23, 2018, 14:00 UTC · lore

This small patch series makes it easy to spot big performance regressions, so that they can later be investigated.

For example:

$ ./aggregate.perl --sortbyregression --subsection "without libpcre" v2.14.3 v2.15.1 v2.16.2 p4220-log-grep-engines.sh +5.0% p4220-log-grep-engines.2 0.60(0.58+0.02) 0.63(0.59+0.04) v2.14.3 v2.15.1 +4.5% p4220-log-grep-engines.10 0.67(0.64+0.03) 0.70(0.67+0.02) v2.14.3 v2.15.1 +1.7% p4220-log-grep-engines.5 0.58(0.57+0.01) 0.59(0.59+0.00) v2.14.3 v2.15.1 +1.7% p4220-log-grep-engines.6 0.58(0.58+0.00) 0.59(0.56+0.02) v2.14.3 v2.15.1 +1.7% p4220-log-grep-engines.17 0.58(0.57+0.01) 0.59(0.56+0.02) v2.14.3 v2.15.1 +1.7% p4220-log-grep-engines.1 0.60(0.58+0.01) 0.61(0.60+0.01) v2.14.3 v2.15.1 +1.6% p4220-log-grep-engines.13 0.64(0.63+0.02) 0.65(0.63+0.01) v2.14.3 v2.15.1 +1.5% p4220-log-grep-engines.9 0.67(0.66+0.01) 0.68(0.67+0.01) v2.14.3 v2.15.1 +0.0% p4220-log-grep-engines.14 0.65(0.62+0.02) 0.65(0.63+0.02) v2.14.3 v2.15.1 +0.0% p4220-log-grep-engines.18 0.58(0.57+0.00) 0.58(0.56+0.02) v2.14.3 v2.15.1 -1.5% p4220-log-grep-engines.13 0.65(0.63+0.01) 0.64(0.62+0.01) v2.15.1 v2.16.2 -1.5% p4220-log-grep-engines.14 0.65(0.63+0.02) 0.64(0.60+0.03) v2.15.1 v2.16.2 -1.6% p4220-log-grep-engines.1 0.61(0.60+0.01) 0.60(0.58+0.02) v2.15.1 v2.16.2 -1.7% p4220-log-grep-engines.5 0.59(0.59+0.00) 0.58(0.55+0.02) v2.15.1 v2.16.2 -1.7% p4220-log-grep-engines.6 0.59(0.56+0.02) 0.58(0.55+0.02) v2.15.1 v2.16.2 -1.7% p4220-log-grep-engines.18 0.58(0.56+0.02) 0.57(0.55+0.02) v2.15.1 v2.16.2 -2.9% p4220-log-grep-engines.9 0.68(0.67+0.01) 0.66(0.64+0.02) v2.15.1 v2.16.2 -3.4% p4220-log-grep-engines.17 0.59(0.56+0.02) 0.57(0.55+0.01) v2.15.1 v2.16.2 -4.3% p4220-log-grep-engines.10 0.70(0.67+0.02) 0.67(0.66+0.01) v2.15.1 v2.16.2 -4.8% p4220-log-grep-engines.2 0.63(0.59+0.04) 0.60(0.57+0.03) v2.15.1 v2.16.2

Christian Couder (2):
  perf/aggregate: add display_dir()
  perf/aggregate: add --sortbyregression option

 t/perf/aggregate.perl | 59 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 5 deletions(-)

-- 
2.17.0.rc0.37.g8f476fabe9

Christian Couder· Mar 23, 2018, 14:00 UTC · re: Christian Couder · lore

[PATCH v1 1/2] perf/aggregate: add display_dir()

This new helper function will be reused in a subsequent commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/perf/aggregate.perl | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Show changes to t/perf/aggregate.perl +7 −4

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index 821cf1498b..b9c0e3243d 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -147,6 +147,11 @@ sub have_slash {
 	return 0;
 }
 
+sub display_dir {
+    my ($d) = @_;
+    return exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d};
+}
+
 sub print_default_results {
 	my %descrs;
 	my $descrlen = 4; # "Test"
@@ -168,8 +173,7 @@ sub print_default_results {
 	my %times;
 	my @colwidth = ((0)x@dirs);
 	for my $i (0..$#dirs) {
-		my $d = $dirs[$i];
-		my $w = length (exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d});
+		my $w = length display_dir($dirs[$i]);
 		$colwidth[$i] = $w if $w > $colwidth[$i];
 	}
 	for my $t (@subtests) {
@@ -188,8 +192,7 @@ sub print_default_results {
 
 	printf "%-${descrlen}s", "Test";
 	for my $i (0..$#dirs) {
-		my $d = $dirs[$i];
-		printf "   %-$colwidth[$i]s", (exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d});
+		printf "   %-$colwidth[$i]s", display_dir($dirs[$i]);
 	}
 	print "\n";
 	print "-"x$totalwidth, "\n";

-- 
2.17.0.rc0.37.g8f476fabe9

Christian Couder· Mar 23, 2018, 14:00 UTC · re: Christian Couder · lore

[PATCH v1 2/2] perf/aggregate: add --sortbyregression option

One of the most interesting thing one can be interested in when looking at performance test results is possible performance regressions.

This new option makes it easy to spot such possible regressions.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/perf/aggregate.perl | 48 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

Show changes to t/perf/aggregate.perl +47 −1

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index b9c0e3243d..9d032b286e 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -37,7 +37,7 @@ sub format_times {
 }
 
 my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
-    $codespeed, $subsection, $reponame);
+    $codespeed, $sortbyregression, $subsection, $reponame);
 while (scalar @ARGV) {
 	my $arg = $ARGV[0];
 	my $dir;
@@ -46,6 +46,11 @@ while (scalar @ARGV) {
 		shift @ARGV;
 		next;
 	}
+	if ($arg eq "--sortbyregression") {
+		$sortbyregression = 1;
+		shift @ARGV;
+		next;
+	}
 	if ($arg eq "--subsection") {
 		shift @ARGV;
 		$subsection = $ARGV[0];
@@ -209,6 +214,45 @@ sub print_default_results {
 	}
 }
 
+sub print_sortbyregression_results {
+	my ($subsection) = @_;
+
+	my @evolutions;
+	for my $t (@subtests) {
+		my ($prevr, $prevu, $prevs, $prevrev);
+		for my $i (0..$#dirs) {
+			my $d = $dirs[$i];
+			my ($r, $u, $s) = get_times("$resultsdir/$prefixes{$d}$t.times");
+			if ($i > 0 and defined $r and defined $prevr and $prevr > 0) {
+			    my $percent = 100.0 * ($r - $prevr) / $prevr;
+			    push @evolutions, { "percent"  => $percent,
+						"test"     => $t,
+						"prevrev"  => $prevrev,
+						"rev"      => $d,
+						"prevr"    => $prevr,
+						"r"        => $r,
+						"prevu"    => $prevu,
+						"u"        => $u,
+						"prevs"    => $prevs,
+						"s"        => $s};
+			}
+			($prevr, $prevu, $prevs, $prevrev) = ($r, $u, $s, $d);
+		}
+	}
+
+	my @sorted_evolutions = sort { $b->{percent} <=> $a->{percent} } @evolutions;
+
+	for my $e (@sorted_evolutions) {
+	    printf "%+.1f%%", $e->{percent};
+	    print " " . $e->{test};
+	    print " " . format_times($e->{prevr}, $e->{prevu}, $e->{prevs});
+	    print " " . format_times($e->{r}, $e->{u}, $e->{s});
+	    print " " . display_dir($e->{prevrev});
+	    print " " . display_dir($e->{rev});
+	    print "\n";
+	}
+}
+
 sub print_codespeed_results {
 	my ($subsection) = @_;
 
@@ -263,6 +307,8 @@ binmode STDOUT, ":utf8" or die "PANIC on binmode: $!";
 
 if ($codespeed) {
 	print_codespeed_results($subsection);
+} elsif ($sortbyregression) {
+	print_sortbyregression_results($subsection);
 } else {
 	print_default_results();
 }

-- 
2.17.0.rc0.37.g8f476fabe9

Christian Couder· Mar 23, 2018, 14:08 UTC · re: Christian Couder · lore

Re: [PATCH v1 0/2] perf/aggregate: sort result by regression

On Fri, Mar 23, 2018 at 3:00 PM, Christian Couder <christian.couder@gmail.com> wrote:

> This small patch series makes it easy to spot big performance
> regressions, so that they can later be investigated.

Sorry I just realized there are indent problems in the patches.

Junio C Hamano· Mar 23, 2018, 21:16 UTC · re: Christian Couder · lore

Re: [PATCH v1 0/2] perf/aggregate: sort result by regression

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

Show 6 quoted lines

> This small patch series makes it easy to spot big performance
> regressions, so that they can later be investigated.
>
> For example:
>
> $ ./aggregate.perl --sortbyregression --subsection "without libpcre" v2.14.3 v2.15.1 v2.16.2 p4220-log-grep-engines.sh

Are we comfortable with the idea that other kinds of sorting, when invented in the future, would have to say

    $ ./aggregate.perl --sortbysomethingelse --subsection "without libpcre" \
        A B C p4220-log-grep-engines.sh

or will we regret that and wish if we could write it as

    $ ./aggregate.perl --sort-by=somethingelse --subsection "without libpcre" \
        A B C p4220-log-grep-engines.sh

If the latter, perhaps we should use --soft-by=regression from day one.

Do we expect that "taking a lot more more rtime than the previous" will stay to be the only kind of "regression" we care about in the context of t/perf? If so, there is no need for further suggestion, but if not, perhaps we should plan if/how we could also parameterize the "rtime" part from the command line. E.g.

    $ ./aggregate.perl --sort-by=regression:stime

might be a way to say "we only care about the stime part" in the future, even though --sort-by=regression may be a short-hand to say "we care about rtime regression" i.e. "--sort-by=regression:rtime".

Christian Couder· Mar 24, 2018, 07:05 UTC · re: Junio C Hamano · lore

Re: [PATCH v1 0/2] perf/aggregate: sort result by regression

On Fri, Mar 23, 2018 at 10:16 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> This small patch series makes it easy to spot big performance
>> regressions, so that they can later be investigated.
>>
>> For example:
>>
>> $ ./aggregate.perl --sortbyregression --subsection "without libpcre" v2.14.3 v2.15.1 v2.16.2 p4220-log-grep-engines.sh
>
> Are we comfortable with the idea that other kinds of sorting, when
> invented in the future, would have to say
>
>     $ ./aggregate.perl --sortbysomethingelse --subsection "without libpcre" \
>         A B C p4220-log-grep-engines.sh
>
> or will we regret that and wish if we could write it as
>
>     $ ./aggregate.perl --sort-by=somethingelse --subsection "without libpcre" \
>         A B C p4220-log-grep-engines.sh
>
> If the latter, perhaps we should use --soft-by=regression from day one.

Yeah, I think it is probably better to use --sort-by=regression (not --soft-by ;-), so I will use that in the next version.

Show 11 quoted lines

> Do we expect that "taking a lot more more rtime than the previous"
> will stay to be the only kind of "regression" we care about in the
> context of t/perf?  If so, there is no need for further suggestion,
> but if not, perhaps we should plan if/how we could also parameterize
> the "rtime" part from the command line.  E.g.
>
>     $ ./aggregate.perl --sort-by=regression:stime
>
> might be a way to say "we only care about the stime part" in the
> future, even though --sort-by=regression may be a short-hand to say
> "we care about rtime regression" i.e. "--sort-by=regression:rtime".

Yeah, I think we can have the short form "--sort-by=regression" mean "--sort-by=regression:rtime" and skip implementing the long form. I will talk about the long form in the commit message.