Re: [PATCH 9/9] trace2: add global counters
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 20, 2021, 17:14 UTC
- Message-ID
- <211220.868rwfur46.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <596caede216c44d2a01358229bc1a47248e708b1.1640012469.git.gitgitgadget@gmail.com>
On Mon, Dec 20 2021, Jeff Hostetler via GitGitGadget wrote:
Show 7 quoted lines
> From: Jeff Hostetler <jeffhost@microsoft.com>
> [...]
> +struct ut_011_data {
> + int v1, v2;
> +};
> [...]
> + struct ut_011_data data = { 0, 0 };Nit: Just "{ 0 }" is OK for zero'd out initialization. No need to keep
extending this for every field.For things that want to exhaustively list fields for clarity, designated
would be preferred:
o
{ .v1 = 0, .v2 = 0 }Show 12 quoted lines
> + 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.
Show 14 quoted lines
> +# 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>Show 7 quoted lines
> +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 }".