diff mbox series

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

Message ID 20230811053940.1408424-15-rostedt@goodmis.org (mailing list archive)
State Superseded
Headers show
Series libtraceeval histogram: Updates | expand

Commit Message

Steven Rostedt Aug. 11, 2023, 5:39 a.m. UTC 
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(-)

Comments

Ross Zwisler Aug. 16, 2023, 10:37 p.m. UTC | #1 
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
>
Steven Rostedt Aug. 16, 2023, 11:12 p.m. UTC | #2 
On Wed, 16 Aug 2023 16:37:54 -0600
Ross Zwisler <zwisler@google.com> wrote:

> 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.
> 

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
> >
Steven Rostedt Aug. 17, 2023, 1:03 a.m. UTC | #3 
On Wed, 16 Aug 2023 19:12:25 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 16 Aug 2023 16:37:54 -0600
> Ross Zwisler <zwisler@google.com> wrote:
> 
> > 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.
> >   
> 
> 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.

Anyway, it this does not belong in this patch. The return value of
traceeval_insert() change should be a separate patch. One that I will add
in another series after this one.

-- Steve
Steven Rostedt Aug. 17, 2023, 1:13 a.m. UTC | #4 
On Wed, 16 Aug 2023 19:12:25 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > > +
> > > +	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 realized I actually had this kind of backwards. On failure, we want to put
back old and remove the data added to vals (aka copy).

Here's what I'm doing:

	for (i = 0; i < size; i++) {
		old[i] = copy[i];

		if (copy_traceeval_data(types + i, stats + i,
					vals + i, copy + i))
			goto fail;
	}
	data_release(size, old, types);
	return 0;
 fail:
	/* Free the new values that were added */
	data_release(i, copy, types);
	/* Put back the old values */
	for (i--; i >= 0; i--) {
		copy_traceeval_data(types + i, NULL,
				    copy + i, old + i);
	}
	return -1;

-- Steve
diff mbox series

Patch

diff --git a/src/histograms.c b/src/histograms.c
index 8c73d8cd9f45..643a550422f6 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -598,17 +598,23 @@  static int copy_traceeval_data(struct traceeval_type *type,
  *
  * Does not call release() if a copy() exists.
  */
-static void data_release(size_t size, union traceeval_data **data,
-				struct traceeval_type *type)
+static void data_release(size_t size, union traceeval_data *data,
+			 struct traceeval_type *type)
 {
 	for (size_t i = 0; i < size; i++) {
 		/* A copy should handle releases */
 		if (type[i].release && !type[i].copy)
-			type[i].release(&type[i], &(*data)[i]);
+			type[i].release(&type[i], &data[i]);
 
 		if (type[i].type == TRACEEVAL_TYPE_STRING)
-			free((*data)[i].string);
+			free(data[i].string);
 	}
+}
+
+static void data_release_and_free(size_t size, union traceeval_data **data,
+				struct traceeval_type *type)
+{
+	data_release(size, *data, type);
 	free(*data);
 	*data = NULL;
 }
@@ -642,7 +648,7 @@  static int copy_traceeval_data_set(size_t size, struct traceeval_type *type,
 	return 1;
 
 fail:
-	data_release(i, copy, type);
+	data_release_and_free(i, copy, type);
 	return -1;
 }
 
@@ -701,7 +707,7 @@  void traceeval_results_release(struct traceeval *teval,
 		return;
 	}
 
-	data_release(teval->nr_val_types, &results, teval->val_types);
+	data_release_and_free(teval->nr_val_types, &results, teval->val_types);
 }
 
 static struct entry *create_hist_entry(struct traceeval *teval,
@@ -757,7 +763,7 @@  static int create_entry(struct traceeval *teval,
 	return 0;
 
 fail:
-	data_release(teval->nr_key_types, &new_keys, teval->key_types);
+	data_release_and_free(teval->nr_key_types, &new_keys, teval->key_types);
 
 fail_stats:
 	free(entry->val_stats);
@@ -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.
  */
 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;
+
+	for (i = 0; i < size; i++) {
+		old[i] = copy[i];
+
+		if (copy_traceeval_data(types + i, stats + i,
+					vals + i, copy + i))
+			goto fail;
+	}
 
-	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,