diff mbox series

[v3,14/18] libtraceeval histogram: Use stack for old copy in update

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

Commit Message

Steven Rostedt Aug. 17, 2023, 1:33 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 | 51 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 13 deletions(-)

Comments

Ross Zwisler Aug. 17, 2023, 7:29 p.m. UTC | #1 
On Wed, Aug 16, 2023 at 09:33:06PM -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 | 51 ++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/src/histograms.c b/src/histograms.c
> index 57b4b2166e3b..3050bad4190e 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
<>
> @@ -776,15 +782,34 @@ fail_entry:
>  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 0;
>  
> -	clean_data_set(entry->vals, teval->val_types, teval->nr_val_types);
> -	entry->vals = new_vals;
> +	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);

should be:                          old + i, copy + i);

Right now we're copying 'copy' into 'old', but we want to be doing it the
other way around so we restore 'copy' back to its saved state in 'old'

> +	}
> +	return -1;
>  }
>  
>  struct traceeval_stat *traceeval_stat(struct traceeval *teval,
> -- 
> 2.40.1
>
Steven Rostedt Aug. 17, 2023, 7:42 p.m. UTC | #2 
On Thu, 17 Aug 2023 13:29:26 -0600
Ross Zwisler <zwisler@google.com> wrote:

> > + 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);  
> 
> should be:                          old + i, copy + i);
> 
> Right now we're copying 'copy' into 'old', but we want to be doing it the
> other way around so we restore 'copy' back to its saved state in 'old'

I need to change the parameter names to "src" and "dst" as I can't think of
"orig" and "copy" as which direction they go. I looked at that three times,
and still am confused. And probably reverse them too to make it equivalent
to memcpy():

From memcpy() man page;

SYNOPSIS
       #include <string.h>

       void *memcpy(void *restrict dest, const void *restrict src, size_t n);

DESCRIPTION
       The memcpy() function copies n bytes from memory area src to memory
       area dest.  The memory areas must not overlap.  Use memmove(3) if
       the memory areas do overlap.

As most standard libraries have it "dst, src" I'm pretty much thinking that
way, and my "direction" is always dst = src --> dst, src. And that's
exactly what I was thinking above :-p

-- Steve
Ross Zwisler Aug. 17, 2023, 7:44 p.m. UTC | #3 
On Thu, Aug 17, 2023 at 03:42:45PM -0400, Steven Rostedt wrote:
> On Thu, 17 Aug 2023 13:29:26 -0600
> Ross Zwisler <zwisler@google.com> wrote:
> 
> > > + 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);  
> > 
> > should be:                          old + i, copy + i);
> > 
> > Right now we're copying 'copy' into 'old', but we want to be doing it the
> > other way around so we restore 'copy' back to its saved state in 'old'
> 
> I need to change the parameter names to "src" and "dst" as I can't think of
> "orig" and "copy" as which direction they go. 

I was thinking the same thing, 'src' and 'dst' would be better. :)

> I looked at that three times,
> and still am confused. And probably reverse them too to make it equivalent
> to memcpy():
> 
> From memcpy() man page;
> 
> SYNOPSIS
>        #include <string.h>
> 
>        void *memcpy(void *restrict dest, const void *restrict src, size_t n);
> 
> DESCRIPTION
>        The memcpy() function copies n bytes from memory area src to memory
>        area dest.  The memory areas must not overlap.  Use memmove(3) if
>        the memory areas do overlap.
> 
> As most standard libraries have it "dst, src" I'm pretty much thinking that
> way, and my "direction" is always dst = src --> dst, src. And that's
> exactly what I was thinking above :-p
> 
> -- Steve
>
Steven Rostedt Aug. 17, 2023, 8:25 p.m. UTC | #4 
On Thu, 17 Aug 2023 13:44:59 -0600
Ross Zwisler <zwisler@google.com> wrote:

> > I need to change the parameter names to "src" and "dst" as I can't think of
> > "orig" and "copy" as which direction they go.   
> 
> I was thinking the same thing, 'src' and 'dst' would be better. :)

and to make it even more confusing, I made made the compare callback
function ordering that of "copy, orig" ("dst, src")

 typedef int (*traceeval_data_copy_fn)(const struct traceeval_type *type,
                                     union traceeval_data *copy,
                                     const union traceeval_data *origin);

I've just updated my next version to start out by reversing the order of
the copy and to call it "dst" and "src", and I renamed
copy_traceveal_data_set() to dup_traceeval_data_set(), as it's just
duplicating the set, and not really "copying" it (think strdup()), as it
needs to allocate it. That way, I can keep the order, as duplication
variables should come at the end. And this will remove the ambiguity of the
copy order.

-- Steve
diff mbox series

Patch

diff --git a/src/histograms.c b/src/histograms.c
index 57b4b2166e3b..3050bad4190e 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -597,17 +597,23 @@  static int copy_traceeval_data(struct traceeval_type *type,
  *
  * Does not call the release() callback 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;
 }
@@ -641,7 +647,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;
 }
 
@@ -700,7 +706,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,
@@ -756,7 +762,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);
@@ -776,15 +782,34 @@  fail_entry:
 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 0;
 
-	clean_data_set(entry->vals, teval->val_types, teval->nr_val_types);
-	entry->vals = new_vals;
+	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;
 }
 
 struct traceeval_stat *traceeval_stat(struct traceeval *teval,