On Wed, 16 Aug 2023 16:37:54 -0600
Ross Zwisler <zwisler@xxxxxxxxxx> wrote:
> 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.
>
I didn't update the comments well. The idea is, return 1 if the element
already existed, 0 if it did not, and -1 on error. I need to fix this.
> > */
> > 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
Yes, I forgot to add that :-p
> .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.
So the idea is that the release function is called to release (free) the
old data. But the copy may not need to allocate or free, it could use the
existing data. This will be explicitly stated in the man pages (when I get
around to writing them).
-- Steve
>
> >
> > - 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
> >
|