{"thread":{"id":"52296","subject":"[PATCH] perf-lib: remove old result files before running tests","startedAt":"2019-11-19T18:50:59Z","lastAt":"2019-11-25T17:04:19Z","messageCount":7,"participants":["Thomas Gummerer","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"386545","messageId":"20191119185047.8550-1-t.gummerer@gmail.com","threadId":"52296","inReplyTo":null,"subject":"[PATCH] perf-lib: remove old result files before running tests","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-19T18:50:47Z","receivedAt":"2019-11-19T18:50:59Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The perf tests write files recording the results of tests.  These\nresults are later aggregated by 'aggregate.perl'.  If the tests are\nrun multiple times, those results are overwritten by the new results.\nThis works just fine as long as there are only perf tests measuring\nthe times, whose results are stored in \"$base\".times files.\n\nHowever 22bec79d1a (\"t/perf: add infrastructure for measuring sizes\",\n2018-08-17) introduced a new type of test for measuring the size of\nsomething.  The results of this are written to \"$base\".size files.\n\n\"$base\" is essentially made up of the basename of the script plus the\ntest number.  So if test numbers shift because a new test was\nintroduced earlier in the script we might end up with both a \".times\"\nand a \".size\" file for the same test.  In the aggregation script the\n\".times\" file is preferred over the \".size\" file, so some size tests\nmight end with performance numbers from a previous run of the test.\n\nThis is mainly relevant when writing perf tests that check both\nperformance and sizes, and can get quite confusing during\ndevelopement.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n\nThis came out of something different that I'm working on, but makes\nmost sense as a standalone patch, rather than part of that series, so\nI'm sending this out separately.\n\n t/perf/perf-lib.sh | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex b58a43ea43..7e80251889 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -178,10 +178,11 @@ test_wrapper_ () {\n \texport test_prereq\n \tif ! test_skip \"$@\"\n \tthen\n-\t\tbase=$(basename \"$0\" .sh)\n-\t\techo \"$test_count\" >>\"$perf_results_dir\"/$base.subtests\n-\t\techo \"$1\" >\"$perf_results_dir\"/$base.$test_count.descr\n \t\tbase=\"$perf_results_dir\"/\"$PERF_RESULTS_PREFIX$(basename \"$0\" .sh)\".\"$test_count\"\n+\t\trm -f \"$base\".*\n+\t\tno_prefix_base=\"$perf_results_dir\"/$(basename \"$0\" .sh)\n+\t\techo \"$test_count\" >>$no_prefix_base.subtests\n+\t\techo \"$1\" >$no_prefix_base.$test_count.descr\n \t\t\"$test_wrapper_func_\" \"$@\"\n \tfi\n \n-- \n2.24.0.155.gd9f6f3b619\n\n"},{"id":"386610","messageId":"xmqq5zjfchix.fsf@gitster-ct.c.googlers.com","threadId":"52296","inReplyTo":"20191119185047.8550-1-t.gummerer@gmail.com","subject":"Re: [PATCH] perf-lib: remove old result files before running tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-20T04:12:54Z","receivedAt":"2019-11-20T04:13:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> @@ -178,10 +178,11 @@ test_wrapper_ () {\n>  \texport test_prereq\n>  \tif ! test_skip \"$@\"\n>  \tthen\n\n> -\t\tbase=$(basename \"$0\" .sh)\n\nSo we used to use $base to hold the number and the filename here\n\n> -\t\techo \"$test_count\" >>\"$perf_results_dir\"/$base.subtests\n> -\t\techo \"$1\" >\"$perf_results_dir\"/$base.$test_count.descr\n>  \t\tbase=\"$perf_results_dir\"/\"$PERF_RESULTS_PREFIX$(basename \"$0\" .sh)\".\"$test_count\"\n\nand then redefined it to be the results-prefix specific one.\n\n\n> +\t\trm -f \"$base\".*\n\nyou now remove those results-prefix specific one for the $test_count\n(I guess that is as specific you can go) before writing the count\nand the description.  \n\nSo this \"rm -f\" is a no-op when perf-results-prefix is not empty?\n\n> +\t\tno_prefix_base=\"$perf_results_dir\"/$(basename \"$0\" .sh)\n> +\t\techo \"$test_count\" >>$no_prefix_base.subtests\n> +\t\techo \"$1\" >$no_prefix_base.$test_count.descr\n>  \t\t\"$test_wrapper_func_\" \"$@\"\n>  \tfi\n"},{"id":"386618","messageId":"20191120080003.GA38901@cat","threadId":"52296","inReplyTo":"xmqq5zjfchix.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] perf-lib: remove old result files before running tests","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-20T08:00:03Z","receivedAt":"2019-11-20T08:00:09Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 11/20, Junio C Hamano wrote:\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n> \n> > @@ -178,10 +178,11 @@ test_wrapper_ () {\n> >  \texport test_prereq\n> >  \tif ! test_skip \"$@\"\n> >  \tthen\n> \n> > -\t\tbase=$(basename \"$0\" .sh)\n> \n> So we used to use $base to hold the number and the filename here\n> \n> > -\t\techo \"$test_count\" >>\"$perf_results_dir\"/$base.subtests\n> > -\t\techo \"$1\" >\"$perf_results_dir\"/$base.$test_count.descr\n> >  \t\tbase=\"$perf_results_dir\"/\"$PERF_RESULTS_PREFIX$(basename \"$0\" .sh)\".\"$test_count\"\n> \n> and then redefined it to be the results-prefix specific one.\n> \n> \n> > +\t\trm -f \"$base\".*\n> \n> you now remove those results-prefix specific one for the $test_count\n> (I guess that is as specific you can go) before writing the count\n> and the description.  \n> \n> So this \"rm -f\" is a no-op when perf-results-prefix is not empty?\n\nNo, not quite.  It is a no-op the first time a particular test runs\nfor a specific prefix.  The prefix is usually set by the 't/perf/run'\nscript, and indicates the revision that is tested.\n\nSo if we were running for example\n\n    ./run deadbeef... p0001-rev-list.sh\n\nwe'd have a file 'test-results/build_deadbeef....p0001-rev-list.1.times' \nin the t/perf directory.\n\nNow we add a 'test_size' test before the first 'test_perf' test, and\nrun the tests again.  After this run we'd still have that original\nfile from the test results from the previous run, as well as a\n'test-results/build_deadbeef....p0001-rev-list.1.size'.\n\nThis duplicate file at the matching \"$base\" is what we want to avoid.\nThe \"rm -f\" above would remove\n'test-results/build_deadbeef....p0001-rev-list.1.times' so we now only\nhave the '.size' file left, and 'aggregate.perl' gives us the right\nresult.\n\n> > +\t\tno_prefix_base=\"$perf_results_dir\"/$(basename \"$0\" .sh)\n> > +\t\techo \"$test_count\" >>$no_prefix_base.subtests\n> > +\t\techo \"$1\" >$no_prefix_base.$test_count.descr\n> >  \t\t\"$test_wrapper_func_\" \"$@\"\n> >  \tfi\n"},{"id":"386709","messageId":"20191121102042.GA2611@sigill.intra.peff.net","threadId":"52296","inReplyTo":"20191119185047.8550-1-t.gummerer@gmail.com","subject":"Re: [PATCH] perf-lib: remove old result files before running tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-21T10:20:42Z","receivedAt":"2019-11-21T10:20:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 19, 2019 at 06:50:47PM +0000, Thomas Gummerer wrote:\n\n> The perf tests write files recording the results of tests.  These\n> results are later aggregated by 'aggregate.perl'.  If the tests are\n> run multiple times, those results are overwritten by the new results.\n> This works just fine as long as there are only perf tests measuring\n> the times, whose results are stored in \"$base\".times files.\n> \n> However 22bec79d1a (\"t/perf: add infrastructure for measuring sizes\",\n> 2018-08-17) introduced a new type of test for measuring the size of\n> something.  The results of this are written to \"$base\".size files.\n> \n> \"$base\" is essentially made up of the basename of the script plus the\n> test number.  So if test numbers shift because a new test was\n> introduced earlier in the script we might end up with both a \".times\"\n> and a \".size\" file for the same test.  In the aggregation script the\n> \".times\" file is preferred over the \".size\" file, so some size tests\n> might end with performance numbers from a previous run of the test.\n> \n> This is mainly relevant when writing perf tests that check both\n> performance and sizes, and can get quite confusing during\n> developement.\n\nThe problem description makes sense to me.\n\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index b58a43ea43..7e80251889 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -178,10 +178,11 @@ test_wrapper_ () {\n>  \texport test_prereq\n>  \tif ! test_skip \"$@\"\n>  \tthen\n> -\t\tbase=$(basename \"$0\" .sh)\n> -\t\techo \"$test_count\" >>\"$perf_results_dir\"/$base.subtests\n> -\t\techo \"$1\" >\"$perf_results_dir\"/$base.$test_count.descr\n>  \t\tbase=\"$perf_results_dir\"/\"$PERF_RESULTS_PREFIX$(basename \"$0\" .sh)\".\"$test_count\"\n> +\t\trm -f \"$base\".*\n> +\t\tno_prefix_base=\"$perf_results_dir\"/$(basename \"$0\" .sh)\n> +\t\techo \"$test_count\" >>$no_prefix_base.subtests\n> +\t\techo \"$1\" >$no_prefix_base.$test_count.descr\n>  \t\t\"$test_wrapper_func_\" \"$@\"\n\nI had a little trouble following the patch because of a few things:\n\n  - the reordering of earlier lines. We don't care about subtests at\n    all, and that line could stay the same. But we do have to reorder\n    the \"descr\" one because of the broad wildcard in the \"rm\". That\n    could be narrowed, but I guess you wanted to future-proof it against\n    new types.\n\n  - the $no_prefix_base variable differs from $base not just in lacking\n    the prefix, but also in lacking $test_count at the end.\n\nSo I think it's doing the right thing overall, though there is one bug:\n$no_prefix_base is not quoted, but could contain spaces due to\n$perf_results_dir. I think that would cause bash to complain if there's\na space in your path.\n\nBut I wonder if it would be simpler to just always use the same file for\nthe test result, overwriting it each time, and let the reader figure out\nthe type. The aggregate script's get_times() already uses a regex to\ndistinguish the two. That's enough for the two types we have, and we\ncould later add a header line if it becomes necessary.\n\nSomething like the patch below. That removes any confusion about cruft\nfiles being left behind, or which file should be preferred, etc.\n\nThe diff would be even smaller if we just kept calling it \"times\", but\nthat's probably unnecessarily confusing.\n\n-Peff\n\n---\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex 66554d2161..112fc23dbe 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -219,13 +219,7 @@ sub print_default_results {\n \t\tfor my $i (0..$#dirs) {\n \t\t\tmy $d = $dirs[$i];\n \t\t\tmy $base = \"$resultsdir/$prefixes{$d}$t\";\n-\t\t\t$times{$prefixes{$d}.$t} = [];\n-\t\t\tforeach my $type (qw(times size)) {\n-\t\t\t\tif (-e \"$base.$type\") {\n-\t\t\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$base.$type\")];\n-\t\t\t\t\tlast;\n-\t\t\t\t}\n-\t\t\t}\n+\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$base.result\")];\n \t\t\tmy ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};\n \t\t\tmy $w = length format_times($r,$u,$s,$firstr);\n \t\t\t$colwidth[$i] = $w if $w > $colwidth[$i];\n@@ -267,7 +261,7 @@ sub print_sorted_results {\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\tmy ($r, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.result\");\n \t\t\tif ($i > 0 and defined $r and defined $prevr and $prevr > 0) {\n \t\t\t\tmy $percent = 100.0 * ($r - $prevr) / $prevr;\n \t\t\t\tpush @evolutions, { \"percent\"  => $percent,\n@@ -327,7 +321,7 @@ sub print_codespeed_results {\n \t\t\tmy $commitid = $prefixes{$d};\n \t\t\t$commitid =~ s/^build_//;\n \t\t\t$commitid =~ s/\\.$//;\n-\t\t\tmy ($result_value, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.times\");\n+\t\t\tmy ($result_value, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.result\");\n \n \t\t\tmy %vals = (\n \t\t\t\t\"commitid\" => $commitid,\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex b58a43ea43..13e389367a 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -214,7 +214,7 @@ test_perf_ () {\n \telse\n \t\ttest_ok_ \"$1\"\n \tfi\n-\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".times\n+\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".result\n }\n \n test_perf () {\n@@ -223,7 +223,7 @@ test_perf () {\n \n test_size_ () {\n \tsay >&3 \"running: $2\"\n-\tif test_eval_ \"$2\" 3>\"$base\".size; then\n+\tif test_eval_ \"$2\" 3>\"$base\".result; then\n \t\ttest_ok_ \"$1\"\n \telse\n \t\ttest_failure_ \"$@\"\n"},{"id":"386808","messageId":"20191122081108.GA38815@cat","threadId":"52296","inReplyTo":"20191121102042.GA2611@sigill.intra.peff.net","subject":"Re: [PATCH] perf-lib: remove old result files before running tests","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-22T08:11:08Z","receivedAt":"2019-11-22T08:12:21Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 11/21, Jeff King wrote:\n> But I wonder if it would be simpler to just always use the same file for\n> the test result, overwriting it each time, and let the reader figure out\n> the type. The aggregate script's get_times() already uses a regex to\n> distinguish the two. That's enough for the two types we have, and we\n> could later add a header line if it becomes necessary.\n> \n> Something like the patch below. That removes any confusion about cruft\n> files being left behind, or which file should be preferred, etc.\n\nYeah, I like what you have below much more than what I had, thanks!\n\n> The diff would be even smaller if we just kept calling it \"times\", but\n> that's probably unnecessarily confusing.\n\nYup, I prefer a more readable end result to a small diff :)\n\n> -Peff\n> \n> ---\n> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\n> index 66554d2161..112fc23dbe 100755\n> --- a/t/perf/aggregate.perl\n> +++ b/t/perf/aggregate.perl\n> @@ -219,13 +219,7 @@ sub print_default_results {\n>  \t\tfor my $i (0..$#dirs) {\n>  \t\t\tmy $d = $dirs[$i];\n>  \t\t\tmy $base = \"$resultsdir/$prefixes{$d}$t\";\n> -\t\t\t$times{$prefixes{$d}.$t} = [];\n> -\t\t\tforeach my $type (qw(times size)) {\n> -\t\t\t\tif (-e \"$base.$type\") {\n> -\t\t\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$base.$type\")];\n> -\t\t\t\t\tlast;\n> -\t\t\t\t}\n> -\t\t\t}\n> +\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$base.result\")];\n>  \t\t\tmy ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};\n>  \t\t\tmy $w = length format_times($r,$u,$s,$firstr);\n>  \t\t\t$colwidth[$i] = $w if $w > $colwidth[$i];\n> @@ -267,7 +261,7 @@ sub print_sorted_results {\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\tmy ($r, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.result\");\n>  \t\t\tif ($i > 0 and defined $r and defined $prevr and $prevr > 0) {\n>  \t\t\t\tmy $percent = 100.0 * ($r - $prevr) / $prevr;\n>  \t\t\t\tpush @evolutions, { \"percent\"  => $percent,\n> @@ -327,7 +321,7 @@ sub print_codespeed_results {\n>  \t\t\tmy $commitid = $prefixes{$d};\n>  \t\t\t$commitid =~ s/^build_//;\n>  \t\t\t$commitid =~ s/\\.$//;\n> -\t\t\tmy ($result_value, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.times\");\n> +\t\t\tmy ($result_value, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.result\");\n>  \n>  \t\t\tmy %vals = (\n>  \t\t\t\t\"commitid\" => $commitid,\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index b58a43ea43..13e389367a 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -214,7 +214,7 @@ test_perf_ () {\n>  \telse\n>  \t\ttest_ok_ \"$1\"\n>  \tfi\n> -\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".times\n> +\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".result\n>  }\n>  \n>  test_perf () {\n> @@ -223,7 +223,7 @@ test_perf () {\n>  \n>  test_size_ () {\n>  \tsay >&3 \"running: $2\"\n> -\tif test_eval_ \"$2\" 3>\"$base\".size; then\n> +\tif test_eval_ \"$2\" 3>\"$base\".result; then\n>  \t\ttest_ok_ \"$1\"\n>  \telse\n>  \t\ttest_failure_ \"$@\"\n"},{"id":"386990","messageId":"20191125140925.GC494@sigill.intra.peff.net","threadId":"52296","inReplyTo":"20191122081108.GA38815@cat","subject":"Re: [PATCH] perf-lib: remove old result files before running tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-25T14:09:25Z","receivedAt":"2019-11-25T14:09:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 22, 2019 at 08:11:08AM +0000, Thomas Gummerer wrote:\n\n> On 11/21, Jeff King wrote:\n> > But I wonder if it would be simpler to just always use the same file for\n> > the test result, overwriting it each time, and let the reader figure out\n> > the type. The aggregate script's get_times() already uses a regex to\n> > distinguish the two. That's enough for the two types we have, and we\n> > could later add a header line if it becomes necessary.\n> > \n> > Something like the patch below. That removes any confusion about cruft\n> > files being left behind, or which file should be preferred, etc.\n> \n> Yeah, I like what you have below much more than what I had, thanks!\n> \n> > The diff would be even smaller if we just kept calling it \"times\", but\n> > that's probably unnecessarily confusing.\n> \n> Yup, I prefer a more readable end result to a small diff :)\n\nOK, here it is wrapped up in a commit message. I wasn't sure how to\nattribute it, since you really did most of the interesting work and I\njust swooped in with an alternative. I'm happy to make you the author,\nbut I didn't want you to get blamed for my bugs. ;)\n\nJunio, this would replace tg/perf-remove-stale-result.\n\n-- >8 --\nSubject: [PATCH] perf-lib: use a single filename for all measurement types\n\nThe perf tests write files recording the results of tests.  These\nresults are later aggregated by 'aggregate.perl'.  If the tests are run\nmultiple times, those results are overwritten by the new results.  This\nworks just fine as long as there are only perf tests measuring the\ntimes, whose results are stored in \"$base\".times files.\n\nHowever 22bec79d1a (\"t/perf: add infrastructure for measuring sizes\",\n2018-08-17) introduced a new type of test for measuring the size of\nsomething.  The results of this are written to \"$base\".size files.\n\n\"$base\" is essentially made up of the basename of the script plus the\ntest number.  So if test numbers shift because a new test was\nintroduced earlier in the script we might end up with both a \".times\"\nand a \".size\" file for the same test.  In the aggregation script the\n\".times\" file is preferred over the \".size\" file, so some size tests\nmight end with performance numbers from a previous run of the test.\n\nThis is mainly relevant when writing perf tests that check both\nperformance and sizes, and can get quite confusing during\ndevelopement.\n\nWe could fix this by doing a more thorough job of cleaning out old\n\".times\" and \".size\" files before running each test. However, an even\neasier solution is to just use the same filename for both types of\nmeasurement, meaning we'll always overwrite the previous result. We\ndon't even need to change the file format to distinguish the two;\naggregate.perl already decides which is which based on a regex of the\ncontent (this may become ambiguous if we add new types in the future,\nbut we could easily add a header field to the file at that point).\n\nBased on an initial patch from Thomas Gummerer, who discovered the\nproblem and did all of the analysis (which I stole for the commit\nmessage above):\n\n  https://public-inbox.org/git/20191119185047.8550-1-t.gummerer@gmail.com/\n\nHelped-by: Thomas Gummerer <t.gummerer@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/perf/aggregate.perl | 12 +++---------\n t/perf/perf-lib.sh    |  4 ++--\n 2 files changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex 66554d2161..112fc23dbe 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -219,13 +219,7 @@ sub print_default_results {\n \t\tfor my $i (0..$#dirs) {\n \t\t\tmy $d = $dirs[$i];\n \t\t\tmy $base = \"$resultsdir/$prefixes{$d}$t\";\n-\t\t\t$times{$prefixes{$d}.$t} = [];\n-\t\t\tforeach my $type (qw(times size)) {\n-\t\t\t\tif (-e \"$base.$type\") {\n-\t\t\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$base.$type\")];\n-\t\t\t\t\tlast;\n-\t\t\t\t}\n-\t\t\t}\n+\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$base.result\")];\n \t\t\tmy ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};\n \t\t\tmy $w = length format_times($r,$u,$s,$firstr);\n \t\t\t$colwidth[$i] = $w if $w > $colwidth[$i];\n@@ -267,7 +261,7 @@ sub print_sorted_results {\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\tmy ($r, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.result\");\n \t\t\tif ($i > 0 and defined $r and defined $prevr and $prevr > 0) {\n \t\t\t\tmy $percent = 100.0 * ($r - $prevr) / $prevr;\n \t\t\t\tpush @evolutions, { \"percent\"  => $percent,\n@@ -327,7 +321,7 @@ sub print_codespeed_results {\n \t\t\tmy $commitid = $prefixes{$d};\n \t\t\t$commitid =~ s/^build_//;\n \t\t\t$commitid =~ s/\\.$//;\n-\t\t\tmy ($result_value, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.times\");\n+\t\t\tmy ($result_value, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.result\");\n \n \t\t\tmy %vals = (\n \t\t\t\t\"commitid\" => $commitid,\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex b58a43ea43..13e389367a 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -214,7 +214,7 @@ test_perf_ () {\n \telse\n \t\ttest_ok_ \"$1\"\n \tfi\n-\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".times\n+\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".result\n }\n \n test_perf () {\n@@ -223,7 +223,7 @@ test_perf () {\n \n test_size_ () {\n \tsay >&3 \"running: $2\"\n-\tif test_eval_ \"$2\" 3>\"$base\".size; then\n+\tif test_eval_ \"$2\" 3>\"$base\".result; then\n \t\ttest_ok_ \"$1\"\n \telse\n \t\ttest_failure_ \"$@\"\n-- \n2.24.0.716.g722aff65ed\n\n"},{"id":"387011","messageId":"20191125170408.GA5044@cat","threadId":"52296","inReplyTo":"20191125140925.GC494@sigill.intra.peff.net","subject":"Re: [PATCH] perf-lib: remove old result files before running tests","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-25T17:04:08Z","receivedAt":"2019-11-25T17:04:19Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 11/25, Jeff King wrote:\n> On Fri, Nov 22, 2019 at 08:11:08AM +0000, Thomas Gummerer wrote:\n> \n> > On 11/21, Jeff King wrote:\n> > > But I wonder if it would be simpler to just always use the same file for\n> > > the test result, overwriting it each time, and let the reader figure out\n> > > the type. The aggregate script's get_times() already uses a regex to\n> > > distinguish the two. That's enough for the two types we have, and we\n> > > could later add a header line if it becomes necessary.\n> > > \n> > > Something like the patch below. That removes any confusion about cruft\n> > > files being left behind, or which file should be preferred, etc.\n> > \n> > Yeah, I like what you have below much more than what I had, thanks!\n> > \n> > > The diff would be even smaller if we just kept calling it \"times\", but\n> > > that's probably unnecessarily confusing.\n> > \n> > Yup, I prefer a more readable end result to a small diff :)\n> \n> OK, here it is wrapped up in a commit message. I wasn't sure how to\n> attribute it, since you really did most of the interesting work and I\n> just swooped in with an alternative. I'm happy to make you the author,\n> but I didn't want you to get blamed for my bugs. ;)\n\nThanks for tying this up.  I wasn't sure how to proceed either, so I'm\nglad you submitted the patch.  And I'm happy to have you as author\nwith the helped-by attribution as you are the one that actually wrote\nthe code :)\n\nI eyeballed the patch again and applied and tested it for good measure\nand it all looks good to me!\n\n> Junio, this would replace tg/perf-remove-stale-result.\n> \n> -- >8 --\n> Subject: [PATCH] perf-lib: use a single filename for all measurement types\n> \n> The perf tests write files recording the results of tests.  These\n> results are later aggregated by 'aggregate.perl'.  If the tests are run\n> multiple times, those results are overwritten by the new results.  This\n> works just fine as long as there are only perf tests measuring the\n> times, whose results are stored in \"$base\".times files.\n> \n> However 22bec79d1a (\"t/perf: add infrastructure for measuring sizes\",\n> 2018-08-17) introduced a new type of test for measuring the size of\n> something.  The results of this are written to \"$base\".size files.\n> \n> \"$base\" is essentially made up of the basename of the script plus the\n> test number.  So if test numbers shift because a new test was\n> introduced earlier in the script we might end up with both a \".times\"\n> and a \".size\" file for the same test.  In the aggregation script the\n> \".times\" file is preferred over the \".size\" file, so some size tests\n> might end with performance numbers from a previous run of the test.\n> \n> This is mainly relevant when writing perf tests that check both\n> performance and sizes, and can get quite confusing during\n> developement.\n> \n> We could fix this by doing a more thorough job of cleaning out old\n> \".times\" and \".size\" files before running each test. However, an even\n> easier solution is to just use the same filename for both types of\n> measurement, meaning we'll always overwrite the previous result. We\n> don't even need to change the file format to distinguish the two;\n> aggregate.perl already decides which is which based on a regex of the\n> content (this may become ambiguous if we add new types in the future,\n> but we could easily add a header field to the file at that point).\n> \n> Based on an initial patch from Thomas Gummerer, who discovered the\n> problem and did all of the analysis (which I stole for the commit\n> message above):\n> \n>   https://public-inbox.org/git/20191119185047.8550-1-t.gummerer@gmail.com/\n> \n> Helped-by: Thomas Gummerer <t.gummerer@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/perf/aggregate.perl | 12 +++---------\n>  t/perf/perf-lib.sh    |  4 ++--\n>  2 files changed, 5 insertions(+), 11 deletions(-)\n> \n> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\n> index 66554d2161..112fc23dbe 100755\n> --- a/t/perf/aggregate.perl\n> +++ b/t/perf/aggregate.perl\n> @@ -219,13 +219,7 @@ sub print_default_results {\n>  \t\tfor my $i (0..$#dirs) {\n>  \t\t\tmy $d = $dirs[$i];\n>  \t\t\tmy $base = \"$resultsdir/$prefixes{$d}$t\";\n> -\t\t\t$times{$prefixes{$d}.$t} = [];\n> -\t\t\tforeach my $type (qw(times size)) {\n> -\t\t\t\tif (-e \"$base.$type\") {\n> -\t\t\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$base.$type\")];\n> -\t\t\t\t\tlast;\n> -\t\t\t\t}\n> -\t\t\t}\n> +\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$base.result\")];\n>  \t\t\tmy ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};\n>  \t\t\tmy $w = length format_times($r,$u,$s,$firstr);\n>  \t\t\t$colwidth[$i] = $w if $w > $colwidth[$i];\n> @@ -267,7 +261,7 @@ sub print_sorted_results {\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\tmy ($r, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.result\");\n>  \t\t\tif ($i > 0 and defined $r and defined $prevr and $prevr > 0) {\n>  \t\t\t\tmy $percent = 100.0 * ($r - $prevr) / $prevr;\n>  \t\t\t\tpush @evolutions, { \"percent\"  => $percent,\n> @@ -327,7 +321,7 @@ sub print_codespeed_results {\n>  \t\t\tmy $commitid = $prefixes{$d};\n>  \t\t\t$commitid =~ s/^build_//;\n>  \t\t\t$commitid =~ s/\\.$//;\n> -\t\t\tmy ($result_value, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.times\");\n> +\t\t\tmy ($result_value, $u, $s) = get_times(\"$resultsdir/$prefixes{$d}$t.result\");\n>  \n>  \t\t\tmy %vals = (\n>  \t\t\t\t\"commitid\" => $commitid,\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index b58a43ea43..13e389367a 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -214,7 +214,7 @@ test_perf_ () {\n>  \telse\n>  \t\ttest_ok_ \"$1\"\n>  \tfi\n> -\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".times\n> +\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".result\n>  }\n>  \n>  test_perf () {\n> @@ -223,7 +223,7 @@ test_perf () {\n>  \n>  test_size_ () {\n>  \tsay >&3 \"running: $2\"\n> -\tif test_eval_ \"$2\" 3>\"$base\".size; then\n> +\tif test_eval_ \"$2\" 3>\"$base\".result; then\n>  \t\ttest_ok_ \"$1\"\n>  \telse\n>  \t\ttest_failure_ \"$@\"\n> -- \n> 2.24.0.716.g722aff65ed\n> \n"}]}