Re: [PATCH 9/9] trace2: add global counters

From: Jeff Hostetler <git@jeffhostetler.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH 9/9] trace2: add global counters
Date: Wed, 22 Dec 2021 17:18:35 -0500	[thread overview]
Message-ID: <db155798-eb74-1ddf-d932-1243e76bd2ae@jeffhostetler.com> (raw)
In-Reply-To: <211220.868rwfur46.gmgdl@evledraar.gmail.com>

On 12/20/21 12:14 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Dec 20 2021, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>> [...]

>> +	int nr_threads = 0;
>> +	int k;
>> +	pthread_t *pids = NULL;
>> +
>> +	if (argc != 3)
>> +		die("%s", usage_error);
>> +	if (get_i(&data.v1, argv[0]))
>> +		die("%s", usage_error);
>> +	if (get_i(&data.v2, argv[1]))
>> +		die("%s", usage_error);
>> +	if (get_i(&nr_threads, argv[2]))
>> +		die("%s", usage_error);
> 
> A partial nit on existing code, as this just extends the pattern, but
> couldn't much of this get_i() etc. just be made redundant by simply
> using the parse-options.c API here?  I.e. OPTION_INTEGER and using named
> arguments would do the validation or you.

I suppose.  It just seemed like a little overkill setting things
up for such a simple and isolated test.  And the cut-n-paste was
quick enough for my purposes.

> 
>> +# Exercise the global counter in a loop and confirm that we get the
>> +# expected sum in an event record.
>> +#
>> +
>> +have_counter_event () {
>> +	thread=$1
>> +	name=$2
>> +	value=$3
>> +	file=$4
>> +
>> +	grep "\"event\":\"counter\".*\"thread\":\"${thread}\".*\"name\":\"${name}\".*\"value\":${value}" $file
>> +
>> +	return $?
>> +}
> 
> It looks like there's no helper, but this is the Nth thing I see where
> wish our "test_region" helper were just a bit more generalized. I.e.:
> 
>      test_trace2 --match=counter --match=thread=$thread --match=name=$name --match=value=$value <trace>
> 
> With test_region just being a wrapper for something like:
> 
>      test_trace2 --match=region_enter --match=category=$category --match=label=$label <trace> &&
>      test_trace2 --match=region_leave --match=category=$category --match=label=$label <trace>

Yes, that would be nice.  But I don't think we should
start that in the middle of this patch series.  Perhaps
you could start a top-level message with a fleshed out
proposal and let everyone discuss it.

> 
>> +static void tr2main_emit_summary_counters(uint64_t us_elapsed_absolute)
>> +{
>> +	struct tr2_tgt *tgt_j;
>> +	int j;
>> +	struct tr2ctr_block merged;
>> +
>> +	memset(&merged, 0, sizeof(merged));
> 
> nit: more memset v.s. "{ 0 }".

Yeah, but lldb wouldn't stop complaining until it was "= { { { 0 } } }"

Jeff