Re: [PATCH 7/9] trace2: add stopwatch timers

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Dec 22, 2021, 21:38 UTC
Message-ID: <9856c7aa-fe07-6164-d101-6d7d25c1ebbb@jeffhostetler.com>
In-Reply-To: <211220.86h7b3usnn.gmgdl@evledraar.gmail.com>

On 12/20/21 11:42 AM, Ævar Arnfjörð Bjarmason wrote:

Show 17 quoted lines

> 
> On Mon, Dec 20 2021, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>> [...]
>> +
>> +void trace2_timer_stop(enum trace2_timer_id tid)
>> +{
>> +	if (!trace2_enabled)
>> +		return;
>> +
>> +	if (tid < 0 || tid >= TRACE2_NUMBER_OF_TIMERS)
>> +		BUG("invalid timer id: %d", tid);
> 
> nit / style: maybe assert() instead for cases where assert() produces
> better info than BUG(). I.e. it would quote the whole expression, and
> show you what condition it violated....

I'd rather leave it a BUG() so that we always get the guard code. assert() goes away in non-debug builds and a little while later "tid" will be used as a subscript.

I'll add the function name to the BUG message to make it a little clearer.

[...]

Show 9 quoted lines

> 
> Perhaps more readable/easily understood as just a (untested):
> 
>      if (!t->interval_count || us_interval >= t->min_us)
> 	    t->min_us = us_interval;
>      if (!t->interval_count || us_interval >= t->max_us)
> 	    t->max_us = us_interval;
> 
> I.e. to avoid duplicating the identical assignment...

[...]

I'll look at something here to make this a little less messy. Probably add a MIN() and MAX() to the mixture.

Show 16 quoted lines

> 
>> +	/*
>> +	 * Number of nested starts on the stack in this thread.  (We
>> +	 * ignore recursive starts and use this to track the recursive
>> +	 * calls.)
>> +	 */
>> +	unsigned int recursion_count;
> 
> Earlier we have various forms of:
> 
>      if (t->recursion_count > 1)
> 
> But since it's unsigned can we just make those a:
> 
>      if (t->recursion_count)
>

The places that are > 0, yes. But the > 1 instances are different since we're counting how many calls are on the stack and want to handle recursive calls differently than the first.