Re: [PATCH] trace2: log progress time and throughput
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jun 23, 2021, 02:55 UTC
- Message-ID
- <YNKiv2Ar77xAxixz@nand.local>
- In-Reply-To
- <871r8w3sxu.fsf@evledraar.gmail.com>
On Mon, Jun 21, 2021 at 03:24:47AM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 15 quoted lines
> > [...]
> > @@ -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