Re: [PATCH v3 4/6] histograms: Add traceeval query

[Steven Rostedt <rostedt@goodmis.org>]

On Tue,  8 Aug 2023 12:11:57 -0400
Stevie Alvarez <stevie.6strings@gmail.com> wrote:

> From: Stevie Alvarez (Google) <stevie.6strings@gmail.com>
> 
> traceeval_query() fetches the values (aka results) of an entry with
> keys that match the array of keys provided. The results of a query can
> be useful for aggregating data between entries/trace events.
> 
> traceeval_results_release() frees the results fetched by the query. The
> user must free the results of a successful query.
> 
> Signed-off-by: Stevie Alvarez (Google) <stevie.6strings@gmail.com>
> ---
>  include/traceeval-hist.h |   6 ++
>  src/histograms.c         | 163 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 169 insertions(+)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index 5203520..4923de1 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -134,4 +134,10 @@ struct traceeval *traceeval_init(const struct traceeval_type *keys,
>  
>  void traceeval_release(struct traceeval *teval);
>  
> +int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
> +		    union traceeval_data **results);
> +
> +void traceeval_results_release(struct traceeval *teval,
> +			       union traceeval_data *results);
> +
>  #endif /* __LIBTRACEEVAL_HIST_H__ */
> diff --git a/src/histograms.c b/src/histograms.c
> index 6fac205..47ff175 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -521,3 +521,166 @@ void traceeval_release(struct traceeval *teval)
>  	teval->val_types = NULL;
>  	free(teval);
>  }
> +
> +/*
> + * Find the entry that @keys corresponds to within @teval.
> + *
> + * Returns 0 on success, 1 if no match found, -1 on error.

Ideally, it would be 1 if found, 0 if not found and -1 on error.

But I guess you did it this way to short circuit the check below, but that
doesn't match either.

> + */
> +static int get_entry(struct traceeval *teval, const union traceeval_data *keys,
> +		     struct entry **result)
> +{
> +	struct hist_table *hist;
> +	int i;
> +	int check;

Upside-down x-mas tree format :-)

	struct hist_table *hist;
	struct entry;
	int check = 0;
	int i;

There's a reason I set check to zero.

> +
> +	if (!teval || !keys)
> +		return -1;
> +
> +	hist = teval->hist;


> +	i = 0;
> +	for (struct entry *entry = hist->map; i < hist->nr_entries;

Do not declare complex variables in for loops. Makes them much harder to
read.

	for (i = 0, entry = hist->map; i < hist->nr_entries; entry = &hist->map[++i])

> +			entry = &hist->map[++i]) {

Probably want to optimize this with a hash anyway, especially since the
code will likely have hundreds of keys and do thousands of lookups.

> +		check = compare_traceeval_data_set(entry->keys, keys,

compare_traceeval_data_set() should return 1 on found, zero on not found
and -1 on error.

> +				teval->key_types, teval->nr_key_types);
> +

We can then have:

		if (!check)
			continue;

		break;

> +		/* return entry if keys match */
> +		if (!check) {
> +			*result = entry;
> +			return 0;
> +		} else if (check == 1) {
> +			continue;
> +		} else {
> +			return check;
> +		}
> +	}

	if (check > 0)
		*result = entry;

	return check;

> +
> +	return 1;
> +}
> +
> +/*
> + * Copy @orig to @copy with respect to @type.
> + *
> + * Return 0 on success, -1 on error.
> + */
> +static int copy_traceeval_data(struct traceeval_type *type,
> +				const union traceeval_data *orig,
> +				union traceeval_data *copy)
> +{
> +	*copy = *orig;
> +
> +	switch (type->type) {
> +	case TRACEEVAL_TYPE_STRING:
> +		copy->string = NULL;
> +		if (orig->string)
> +			copy->string = strdup(orig->string);
> +		if (!copy->string)
> +			return -1;
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Free @data with respect to @size and @type.
> + *
> + * Does not call dyn_release on type TRACEEVAL_TYPE_DYNAMIC.
> + */
> +static void data_release(size_t size, union traceeval_data **data,
> +				struct traceeval_type *type)
> +{
> +	for (size_t i = 0; i < size; i++) {
> +		if (type[i].type == TRACEEVAL_TYPE_STRING)
> +			free((*data)[i].string);
> +	}
> +	free(*data);
> +	*data = NULL;
> +}
> +
> +/*
> + * Copy @orig to @copy with respect to @size and @type.
> + *
> + * Returns 1 on success, -1 on error.
> + */
> +static int copy_traceeval_data_set(size_t size, struct traceeval_type *type,
> +				    const union traceeval_data *orig,
> +				    union traceeval_data **copy)
> +{
> +	size_t i;
> +
> +	*copy = calloc(size, sizeof(**copy));
> +	if (!*copy)
> +		return -1;
> +
> +	for (i = 0; i < size; i++) {
> +		if (copy_traceeval_data(type + i, orig + i, (*copy) + i))
> +			goto fail;
> +	}
> +
> +	return 1;
> +
> +fail:
> +	data_release(i, copy, type);
> +	return -1;
> +}
> +
> +
> +/*
> + * traceeval_query - find the last instance defined by the keys
> + * @teval: The descriptor to search from
> + * @keys: A list of data to look for
> + * @results: A pointer to where to place the results (if found)
> + *
> + * This does a lookup for an instance within the traceeval data.
> + * The @keys is an array defined by the keys declared in traceeval_init().
> + * The @keys will return an item that had the same keys when it was
> + * inserted by traceeval_insert(). The @keys here follow the same rules
> + * as the keys for traceeval_insert().
> + *
> + * Note, when the caller is done with @results, it must call
> + * traceeval_results_release() on it.
> + *
> + * Returns 1 if found, 0 if not found, and -1 on error.

The above is what I would agree on, but does not follow below.

> + */
> +int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
> +		    union traceeval_data **results)
> +{
> +	struct entry *entry;
> +	int check;
> +
> +	if (!teval || !keys || !results)
> +		return -1;
> +
> +	/* find key and copy its corresponding value pair */
> +	if ((check = get_entry(teval, keys, &entry)))

If get_entry() returns 1 on match and 0 if not, and -1 on error, I think we
want this to be:

	if ((check = get_entry(teval, keys, &entry) < 1)
> +		return check;

Right now it returns 1 on not found.

-- Steve

> +	return copy_traceeval_data_set(teval->nr_val_types, teval->val_types,
> +			entry->vals, results);
> +}
> +
> +/*
> + * traceeval_results_release - release the results return by traceeval_query()
> + * @teval: The descriptor used in traceeval_query()
> + * @results: The results returned by traceeval_query()
> + *
> + * The @results returned by traceeval_query() is owned by @teval, and
> + * how it manages it is implementation specific. The caller should not
> + * worry about it. When the caller of traceeval_query() is done with
> + * the @results, it must call traceeval_results_release() on it to
> + * allow traceeval to clean up its references.
> + */
> +void traceeval_results_release(struct traceeval *teval,
> +			       union traceeval_data *results)
> +{
> +	if (!teval || !results) {
> +		if (!results)
> +			print_err("Results to be freed without accompanied traceeval instance!");
> +		return;
> +	}
> +
> +	data_release(teval->nr_val_types, &results, teval->val_types);
> +}