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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 20, 2021, 16:42 UTC
Message-ID: <211220.86h7b3usnn.gmgdl@evledraar.gmail.com>
In-Reply-To: <dd4f0576254defa3ab0e73a73c9c433f28fcca5b.1640012469.git.gitgitgadget@gmail.com>

On Mon, Dec 20 2021, Jeff Hostetler via GitGitGadget wrote:

Show 9 quoted lines

> From: Jeff Hostetler <jeffhost@microsoft.com>
> [...]
> +static void tr2main_emit_summary_timers(uint64_t us_elapsed_absolute)
> +{
> +	struct tr2_tgt *tgt_j;
> +	int j;
> +	struct tr2tmr_block merged;
> +
> +	memset(&merged, 0, sizeof(merged));

Nit: just do a " = { 0 }" assignment above instead.

Show 13 quoted lines

> +	if (tid < 0 || tid >= TRACE2_NUMBER_OF_TIMERS)
> +		BUG("invalid timer id: %d", tid);
> +
> +	tr2tmr_start(tid);
> +}
> +
> +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....

Show 8 quoted lines

> +void tr2tmr_stop(enum trace2_timer_id tid)
> +{
> +	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
> +	struct tr2tmr_timer *t = &ctx->timers.timer[tid];
> +	uint64_t us_now;
> +	uint64_t us_interval;
> +
> +	assert(t->recursion_count > 0);

...as you opted to do here.

Show 19 quoted lines

> +
> +	t->recursion_count--;
> +	if (t->recursion_count > 0)
> +		return; /* still in recursive call */
> +
> +	us_now = getnanotime() / 1000;
> +	us_interval = us_now - t->start_us;
> +
> +	t->total_us += us_interval;
> +
> +	if (!t->interval_count) {
> +		t->min_us = us_interval;
> +		t->max_us = us_interval;
> +	} else {
> +		if (us_interval < t->min_us)
> +			t->min_us = us_interval;
> +		if (us_interval > t->max_us)
> +			t->max_us = us_interval;
> +	}

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...

Show 15 quoted lines

> [...]
> +		if (!t->interval_count)
> +			continue; /* this timer was not used by this thread. */
> +
> +		t_merged->total_us += t->total_us;
> +
> +		if (!t_merged->interval_count) {
> +			t_merged->min_us = t->min_us;
> +			t_merged->max_us = t->max_us;
> +		} else {
> +			if (t->min_us < t_merged->min_us)
> +				t_merged->min_us = t->min_us;
> +			if (t->max_us > t_merged->max_us)
> +				t_merged->max_us = t->max_us;
> +		}

...ditto, maybe since it's used at least twice factor it out to some trivial "static" helper here (maybe not worth it..)>

Show 6 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)