Re: [PATCH v2 14/17] libtraceeval histogram: Use stack for old copy in update

[Ross Zwisler <zwisler@google.com>]

On Fri, Aug 11, 2023 at 01:39:37AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> In the update, instead of using the heap, use the stack to save the old
> values. This should save time where it does not need to allocate and then
> free the value list to do an update.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  src/histograms.c | 48 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
<>
> @@ -772,20 +778,34 @@ fail_entry:
>   *
>   * Frees the old vals field of @entry, unless an error occurs.
>   *
> - * Return 0 on success, -1 on error.
> + * Return 1 on success, -1 on error.

The code is now returning 1, 0 and -1 in different cases, and all three of
these return values are percolating up to be returned by traceeval_insert(),
which is only supposed to return 0 or 1.

>   */
>  static int update_entry(struct traceeval *teval, struct entry *entry,
>  			const union traceeval_data *vals)
>  {
> -	union traceeval_data *new_vals;
> +	struct traceeval_stat *stats = entry->val_stats;
> +	struct traceeval_type *types = teval->val_types;
> +	union traceeval_data *copy = entry->vals;
> +	union traceeval_data old[teval->nr_val_types];
> +	size_t size = teval->nr_val_types;
> +	size_t i;
>  
> -	if (copy_traceeval_data_set(teval->nr_val_types, teval->val_types,
> -				vals, entry->val_stats, &new_vals) == -1)
> -		return -1;
> +	if (!size)
> +		return 1;

If we try and copy a 0 length val, is it okay to just return 0 here and call
it success?

> +
> +	for (i = 0; i < size; i++) {
> +		old[i] = copy[i];
> +
> +		if (copy_traceeval_data(types + i, stats + i,
> +					vals + i, copy + i))
> +			goto fail;
> +	}

I think we still need to rip through old[] and free strings, and also call
.release on types that define it, probably via data_release().

I don't understand why data_release() only calls .release if a .copy function
isn't defined?  Are we saying that .copy() must also release the old data?  If
so that needs to be explicit, but I think we still need to free strings at a
minimum.

>  
> -	clean_data_set(entry->vals, teval->val_types, teval->nr_val_types);
> -	entry->vals = new_vals;
>  	return 0;
> +
> +fail:
> +	data_release(i, old, types);
> +	return -1;
>  }
>  
>  struct traceeval_stat *traceeval_stat(struct traceeval *teval,
> -- 
> 2.40.1
>