On Fri, Aug 11, 2023 at 01:39:37AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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
>
|