Re: [PATCH] trace2: log progress time and throughput

[Taylor Blau <me@ttaylorr.com>]

On Mon, Jun 21, 2021 at 03:24:47AM +0200, Ævar Arnfjörð Bjarmason wrote:
> > [...]
> > @@ -320,6 +321,22 @@ void stop_progress(struct progress **p_progress)
> >  {
> >  	finish_if_sparse(*p_progress);
> >
> > +	if (p_progress && *p_progress) {
> > +		trace2_data_intmax("progress", the_repository, "total_objects",
> > +				   (*p_progress)->total);
>
> We start progress bars for various things in git, yet the trace2 data
> calls every such progress bar with a total "total_objects", even though
> we may not be counting anything to do with objects.
>
> Wouldn't simply s/total_objects/total/ make more sense here, do you rely
> on the name of the current key?

Yeah, I think that the latter of just "total" makes more sense here. I
was going to comment on the fact that "(*p_progress)->total" could be
written simply as "*p_progress->total", but I'm (a) not sure that I
actually prefer the latter to the former, and (b) I find that kind of
style comment generally useless.

But it may make sense to sidestep the whole thing and have a "struct
progress *progress = *p_progress" (that is assigned after we check
p_progress to make sure it's non-NULL) like in stop_progress_msg, which
would clean up a lot of this.

--- 8< ---

Subject: [PATCH] progress.c: avoid repeatedly dereferencing p_progress

stop_progress() takes a double-pointer to a "progress" struct, and
dereferences it twice in each use except one (checking whether
*p_progress is NULL or not).

Mirror the implementation of stop_progress_msg() below by having a local
single-pointer to a progress struct (which is the dereference of
p_progress) to avoid repeatedly dereferencing it.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 progress.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/progress.c b/progress.c
index 680c6a8bf9..390c76b22a 100644
--- a/progress.c
+++ b/progress.c
@@ -319,21 +319,25 @@ static void finish_if_sparse(struct progress *progress)

 void stop_progress(struct progress **p_progress)
 {
+	struct progress *progress;
+
 	if (!p_progress)
 		BUG("don't provide NULL to stop_progress");

-	finish_if_sparse(*p_progress);
+	progress = *p_progress;

-	if (*p_progress) {
+	finish_if_sparse(progress);
+
+	if (progress) {
 		trace2_data_intmax("progress", the_repository, "total_objects",
-				   (*p_progress)->total);
+				   progress->total);

-		if ((*p_progress)->throughput)
+		if (progress->throughput)
 			trace2_data_intmax("progress", the_repository,
 					   "total_bytes",
-					   (*p_progress)->throughput->curr_total);
+					   progress->throughput->curr_total);

-		trace2_region_leave("progress", (*p_progress)->title, the_repository);
+		trace2_region_leave("progress", progress->title, the_repository);
 	}

 	stop_progress_msg(p_progress, _("done"));
--
2.31.1.163.ga65ce7f831