{"thread":{"id":"48125","subject":"[PATCH v1 0/2] perf/aggregate: sort result by regression","startedAt":"2018-03-23T14:00:36Z","lastAt":"2018-03-24T07:05:06Z","messageCount":6,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"342576","messageId":"20180323140007.12096-1-chriscool@tuxfamily.org","threadId":"48125","inReplyTo":null,"subject":"[PATCH v1 0/2] perf/aggregate: sort result by regression","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-23T14:00:05Z","receivedAt":"2018-03-23T14:00:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This small patch series makes it easy to spot big performance\nregressions, so that they can later be investigated.\n\nFor example:\n\n$ ./aggregate.perl --sortbyregression --subsection \"without libpcre\" v2.14.3 v2.15.1 v2.16.2 p4220-log-grep-engines.sh \n+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\n+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\n+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\n+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\n+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\n+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\n+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\n+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\n+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\n+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\n-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\n-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\n-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\n-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\n-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\n-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\n-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\n-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\n-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\n-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\n\nChristian Couder (2):\n  perf/aggregate: add display_dir()\n  perf/aggregate: add --sortbyregression option\n\n t/perf/aggregate.perl | 59 +++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 54 insertions(+), 5 deletions(-)\n\n-- \n2.17.0.rc0.37.g8f476fabe9\n\n"},{"id":"342577","messageId":"20180323140007.12096-2-chriscool@tuxfamily.org","threadId":"48125","inReplyTo":"20180323140007.12096-1-chriscool@tuxfamily.org","subject":"[PATCH v1 1/2] perf/aggregate: add display_dir()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-23T14:00:06Z","receivedAt":"2018-03-23T14:00:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This new helper function will be reused in a subsequent\ncommit.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/perf/aggregate.perl | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex 821cf1498b..b9c0e3243d 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -147,6 +147,11 @@ sub have_slash {\n \treturn 0;\n }\n \n+sub display_dir {\n+    my ($d) = @_;\n+    return exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d};\n+}\n+\n sub print_default_results {\n \tmy %descrs;\n \tmy $descrlen = 4; # \"Test\"\n@@ -168,8 +173,7 @@ sub print_default_results {\n \tmy %times;\n \tmy @colwidth = ((0)x@dirs);\n \tfor my $i (0..$#dirs) {\n-\t\tmy $d = $dirs[$i];\n-\t\tmy $w = length (exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d});\n+\t\tmy $w = length display_dir($dirs[$i]);\n \t\t$colwidth[$i] = $w if $w > $colwidth[$i];\n \t}\n \tfor my $t (@subtests) {\n@@ -188,8 +192,7 @@ sub print_default_results {\n \n \tprintf \"%-${descrlen}s\", \"Test\";\n \tfor my $i (0..$#dirs) {\n-\t\tmy $d = $dirs[$i];\n-\t\tprintf \"   %-$colwidth[$i]s\", (exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d});\n+\t\tprintf \"   %-$colwidth[$i]s\", display_dir($dirs[$i]);\n \t}\n \tprint \"\\n\";\n \tprint \"-\"x$totalwidth, \"\\n\";\n-- \n2.17.0.rc0.37.g8f476fabe9\n\n"},{"id":"342578","messageId":"20180323140007.12096-3-chriscool@tuxfamily.org","threadId":"48125","inReplyTo":"20180323140007.12096-1-chriscool@tuxfamily.org","subject":"[PATCH v1 2/2] perf/aggregate: add --sortbyregression option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-23T14:00:07Z","receivedAt":"2018-03-23T14:00:41Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"One of the most interesting thing one can be interested\nin when looking at performance test results is possible\nperformance regressions.\n\nThis new option makes it easy to spot such possible\nregressions.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/perf/aggregate.perl | 48 ++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex b9c0e3243d..9d032b286e 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -37,7 +37,7 @@ sub format_times {\n }\n \n my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n-    $codespeed, $subsection, $reponame);\n+    $codespeed, $sortbyregression, $subsection, $reponame);\n while (scalar @ARGV) {\n \tmy $arg = $ARGV[0];\n \tmy $dir;\n@@ -46,6 +46,11 @@ while (scalar @ARGV) {\n \t\tshift @ARGV;\n \t\tnext;\n \t}\n+\tif ($arg eq \"--sortbyregression\") {\n+\t\t$sortbyregression = 1;\n+\t\tshift @ARGV;\n+\t\tnext;\n+\t}\n \tif ($arg eq \"--subsection\") {\n \t\tshift @ARGV;\n \t\t$subsection = $ARGV[0];\n@@ -209,6 +214,45 @@ sub print_default_results {\n \t}\n }\n \n+sub print_sortbyregression_results {\n+\tmy ($subsection) = @_;\n+\n+\tmy @evolutions;\n+\tfor my $t (@subtests) {\n+\t\tmy ($prevr, $prevu, $prevs, $prevrev);\n+\t\tfor my $i (0..$#dirs) {\n+\t\t\tmy $d = $dirs[$i];\n+\t\t\tmy ($r, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.times\");\n+\t\t\tif ($i > 0 and defined $r and defined $prevr and $prevr > 0) {\n+\t\t\t    my $percent = 100.0 * ($r - $prevr) / $prevr;\n+\t\t\t    push @evolutions, { \"percent\"  => $percent,\n+\t\t\t\t\t\t\"test\"     => $t,\n+\t\t\t\t\t\t\"prevrev\"  => $prevrev,\n+\t\t\t\t\t\t\"rev\"      => $d,\n+\t\t\t\t\t\t\"prevr\"    => $prevr,\n+\t\t\t\t\t\t\"r\"        => $r,\n+\t\t\t\t\t\t\"prevu\"    => $prevu,\n+\t\t\t\t\t\t\"u\"        => $u,\n+\t\t\t\t\t\t\"prevs\"    => $prevs,\n+\t\t\t\t\t\t\"s\"        => $s};\n+\t\t\t}\n+\t\t\t($prevr, $prevu, $prevs, $prevrev) = ($r, $u, $s, $d);\n+\t\t}\n+\t}\n+\n+\tmy @sorted_evolutions = sort { $b->{percent} <=> $a->{percent} } @evolutions;\n+\n+\tfor my $e (@sorted_evolutions) {\n+\t    printf \"%+.1f%%\", $e->{percent};\n+\t    print \" \" . $e->{test};\n+\t    print \" \" . format_times($e->{prevr}, $e->{prevu}, $e->{prevs});\n+\t    print \" \" . format_times($e->{r}, $e->{u}, $e->{s});\n+\t    print \" \" . display_dir($e->{prevrev});\n+\t    print \" \" . display_dir($e->{rev});\n+\t    print \"\\n\";\n+\t}\n+}\n+\n sub print_codespeed_results {\n \tmy ($subsection) = @_;\n \n@@ -263,6 +307,8 @@ binmode STDOUT, \":utf8\" or die \"PANIC on binmode: $!\";\n \n if ($codespeed) {\n \tprint_codespeed_results($subsection);\n+} elsif ($sortbyregression) {\n+\tprint_sortbyregression_results($subsection);\n } else {\n \tprint_default_results();\n }\n-- \n2.17.0.rc0.37.g8f476fabe9\n\n"},{"id":"342580","messageId":"CAP8UFD2pUwV-GM=qvbJXCW4kcXFoYdjMJU69k+i13q4_x-9bfw@mail.gmail.com","threadId":"48125","inReplyTo":"20180323140007.12096-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 0/2] perf/aggregate: sort result by regression","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-23T14:08:15Z","receivedAt":"2018-03-23T14:08:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Mar 23, 2018 at 3:00 PM, Christian Couder\n<christian.couder@gmail.com> wrote:\n> This small patch series makes it easy to spot big performance\n> regressions, so that they can later be investigated.\n\nSorry I just realized there are indent problems in the patches.\n"},{"id":"342683","messageId":"xmqq7eq2ijc6.fsf@gitster-ct.c.googlers.com","threadId":"48125","inReplyTo":"20180323140007.12096-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 0/2] perf/aggregate: sort result by regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T21:16:09Z","receivedAt":"2018-03-23T21:16:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> This small patch series makes it easy to spot big performance\n> regressions, so that they can later be investigated.\n>\n> For example:\n>\n> $ ./aggregate.perl --sortbyregression --subsection \"without libpcre\" v2.14.3 v2.15.1 v2.16.2 p4220-log-grep-engines.sh \n\nAre we comfortable with the idea that other kinds of sorting, when\ninvented in the future, would have to say\n\n    $ ./aggregate.perl --sortbysomethingelse --subsection \"without libpcre\" \\\n        A B C p4220-log-grep-engines.sh\n\nor will we regret that and wish if we could write it as\n\n    $ ./aggregate.perl --sort-by=somethingelse --subsection \"without libpcre\" \\\n        A B C p4220-log-grep-engines.sh\n\nIf the latter, perhaps we should use --soft-by=regression from day one.\n\nDo we expect that \"taking a lot more more rtime than the previous\"\nwill stay to be the only kind of \"regression\" we care about in the\ncontext of t/perf?  If so, there is no need for further suggestion,\nbut if not, perhaps we should plan if/how we could also parameterize\nthe \"rtime\" part from the command line.  E.g.\n\n    $ ./aggregate.perl --sort-by=regression:stime\n\nmight be a way to say \"we only care about the stime part\" in the\nfuture, even though --sort-by=regression may be a short-hand to say\n\"we care about rtime regression\" i.e. \"--sort-by=regression:rtime\".\n\n"},{"id":"342742","messageId":"CAP8UFD0-GnYtSwNSxM5YQdD+0qAS6R9yW0kFa_88-0Drs2N1ng@mail.gmail.com","threadId":"48125","inReplyTo":"xmqq7eq2ijc6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 0/2] perf/aggregate: sort result by regression","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-03-24T07:05:00Z","receivedAt":"2018-03-24T07:05:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Mar 23, 2018 at 10:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>> This small patch series makes it easy to spot big performance\n>> regressions, so that they can later be investigated.\n>>\n>> For example:\n>>\n>> $ ./aggregate.perl --sortbyregression --subsection \"without libpcre\" v2.14.3 v2.15.1 v2.16.2 p4220-log-grep-engines.sh\n>\n> Are we comfortable with the idea that other kinds of sorting, when\n> invented in the future, would have to say\n>\n>     $ ./aggregate.perl --sortbysomethingelse --subsection \"without libpcre\" \\\n>         A B C p4220-log-grep-engines.sh\n>\n> or will we regret that and wish if we could write it as\n>\n>     $ ./aggregate.perl --sort-by=somethingelse --subsection \"without libpcre\" \\\n>         A B C p4220-log-grep-engines.sh\n>\n> If the latter, perhaps we should use --soft-by=regression from day one.\n\nYeah, I think it is probably better to use --sort-by=regression (not\n--soft-by ;-), so I will use that in the next version.\n\n> Do we expect that \"taking a lot more more rtime than the previous\"\n> will stay to be the only kind of \"regression\" we care about in the\n> context of t/perf?  If so, there is no need for further suggestion,\n> but if not, perhaps we should plan if/how we could also parameterize\n> the \"rtime\" part from the command line.  E.g.\n>\n>     $ ./aggregate.perl --sort-by=regression:stime\n>\n> might be a way to say \"we only care about the stime part\" in the\n> future, even though --sort-by=regression may be a short-hand to say\n> \"we care about rtime regression\" i.e. \"--sort-by=regression:rtime\".\n\nYeah, I think we can have the short form \"--sort-by=regression\" mean\n\"--sort-by=regression:rtime\" and skip implementing the long form. I\nwill talk about the long form in the commit message.\n"}]}