Re: [PATCH 3/8] perf/aggregate: implement codespeed JSON output
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Dec 14, 2017, 08:57 UTC
- Message-ID
- <CAP8UFD1OAwTNAe94DPuJJSAvTa7ojH3sdsK_fkfsW4LUAw+u=Q@mail.gmail.com>
- In-Reply-To
- <xmqqfu8ewepg.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 13, 2017 at 9:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
> Christian Couder <christian.couder@gmail.com> writes:
>
>> my $resultsdir = "test-results";
>> +my $results_section = "";
>> if (exists $ENV{GIT_PERF_SUBSECTION} and $ENV{GIT_PERF_SUBSECTION} ne "") {
>> $resultsdir .= "/" . $ENV{GIT_PERF_SUBSECTION};
>> + $results_section = $ENV{GIT_PERF_SUBSECTION};
>> }
>
> ...
>
>> + my $executable;
>> + if ($results_section eq "") {
>> + $executable = `uname -o -p`;
>> + } else {
>> + $executable = $results_section;
>> + chomp $executable;
>> + }
>
> Aside from portability of 'uname -o' Eric raised, I wonder if the
> platform information is still useful even when perf-subsection is
> specified. With the above code, we can identify that a single
> result is for (say) MacOS only when we are not limiting to a single
> subsection, but wouldn't it be equally a valid desire to be able to
> track performance figures for a single subsection over time and
> being able to say "On MacOS, subsection A's performance dropped
> between release X and X+1 quite a bit, but on Linux x86-64, there
> was no such change" or somesuch?Yeah, I agree that it would be useful. Unfortunately it looks like the number of fields in Codespeed is limited and I am not sure what will be more important for people in general. Another option would be to have "MacOS" in the "environment" field. Or maybe there is a need for a generic way to customize this. For now I just tried to come up with something sensible for what AEvar and me want to do.
Show 17 quoted lines
> IOW, shouldn't the "executable" label always contain the platform
> information, plus an optional subsection info when (and only when)
> the result is limited to a subsection?
>
> By the way, $results_section that is not an empty string at this
> point must have come from $ENV{GIT_PERF_SUBSECTION}, and from the
> way the environment variable is used in t/perf/run, e.g.
>
> (
> GIT_PERF_SUBSECTION="$subsec"
> export GIT_PERF_SUBSECTION
> echo "======== Run for subsection '$GIT_PERF_SUBSECTION' ========"
> run_subsection "$@"
> )
>
> I do not think we expect it to have a trailing LF. What's that
> chomp doing there?It's a silly mistake I made when I reorganized the patches just before sending them. The chomp should be after "$executable = `uname -o -p`;" (so in the other branch of the "if ... else"). I will fix this in the next version.