Re: [PATCH v2 15/17] libtraceeval histogram: Add traceeval_iterator_sort_custom()

[Ross Zwisler <zwisler@google.com>]

On Fri, Aug 11, 2023 at 01:39:38AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add an iterator where the application can supply the sort algorithm where
> it gets the teval descriptor along with the keys and values of both of the
> entries to compare against. Also, allow it to submit its own data to the
> compare function:
> 
> int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
> 				   traceeval_cmp_fn sort_fn, void *data);
> 
> with
> 
> typedef int (*traceeval_cmp_fn)(struct traceeval *teval,
> 				const union traceeval_data *Akeys,
> 				const union traceeval_data *Avals,
> 				const union traceeval_data *Bkeys,
> 				const union traceeval_data *Bvals,
> 				void *data);
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h |  9 +++++++++
>  src/histograms.c         | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index 1a24d6117b93..839f63630897 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -86,6 +86,13 @@ typedef int (*traceeval_data_copy_fn)(const struct traceeval_type *type,
>  				      union traceeval_data *copy,
>  				      const union traceeval_data *origin);
>  
> +typedef int (*traceeval_cmp_fn)(struct traceeval *teval,
> +				const union traceeval_data *Akeys,
> +				const union traceeval_data *Avals,
> +				const union traceeval_data *Bkeys,
> +				const union traceeval_data *Bvals,
> +				void *data);
> +
>  /*
>   * struct traceeval_type - Describes the type of a traceevent_data instance
>   * @type: The enum type that describes the traceeval_data
> @@ -172,6 +179,8 @@ struct traceeval_iterator *traceeval_iterator_get(struct traceeval *teval);
>  void traceeval_iterator_put(struct traceeval_iterator *iter);
>  int traceeval_iterator_sort(struct traceeval_iterator *iter, const char *sort_field,
>  			    int level, bool ascending);
> +int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
> +				   traceeval_cmp_fn sort_fn, void *data);
>  int traceeval_iterator_next(struct traceeval_iterator *iter,
>  			    const union traceeval_data **keys);
>  
> diff --git a/src/histograms.c b/src/histograms.c
> index 643a550422f6..33c87644d468 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -1153,6 +1153,40 @@ static int sort_iter(struct traceeval_iterator *iter)
>  	return 0;
>  }
>  
> +struct iter_custom_data {
> +	struct traceeval_iterator *iter;
> +	traceeval_cmp_fn sort_fn;
> +	void *data;
> +};
> +
> +static int iter_custom_cmp(const void *A, const void *B, void *data)
> +{
> +	struct iter_custom_data *cust_data = data;
> +	struct traceeval_iterator *iter = cust_data->iter;
> +	struct traceeval *teval = iter->teval;
> +	const struct entry *a = *((const struct entry **)A);
> +	const struct entry *b = *((const struct entry **)B);
> +
> +	return cust_data->sort_fn(teval, a->keys, a->vals, b->keys, b->vals,
> +				  cust_data->data);
> +}
> +
> +int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
> +				   traceeval_cmp_fn sort_fn, void *data)
> +{
> +	struct iter_custom_data cust_data = {
> +		.iter = iter,
> +		.sort_fn = sort_fn,
> +		.data = data
> +	};
> +
> +	qsort_r(iter->entries, iter->nr_entries, sizeof(*iter->entries),
> +		iter_custom_cmp, &cust_data);

I guess I don't yet see what this gives us over the existing sorting and
iterators?  Does this do the same thing, we just pass in the sort function
instead of calling traceeval_iterator_sort() one or more times?

> +
> +	iter->needs_sort = false;

Also probably need to set
  iter->next = 0;

> +	return 0;
> +}
> +
>  /**
>   * traceeval_iterator_next - retrieve the next entry from an iterator
>   * @iter: The iterator to retrieve the next entry from
> -- 
> 2.40.1
>