Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 9, 2020, 22:36 UTC
Message-ID: <20200909223631.864145-1-jonathantanmy@google.com>
In-Reply-To: <xmqq365zro2u.fsf@gitster.c.googlers.com>

Show 35 quoted lines

> But there are others that look a bit problematic.  In this example
> taken from fsck, we open all the pack index, only because it is
> needed to show the progress, and the existing conditionals are ways
> to avoid spending unneeded cycles.
> 
> > @@ -836,16 +836,15 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
> >  			uint32_t total = 0, count = 0;
> >  			struct progress *progress = NULL;
> >  
> > -			if (show_progress) {
> > -				for (p = get_all_packs(the_repository); p;
> > -				     p = p->next) {
> > -					if (open_pack_index(p))
> > -						continue;
> > -					total += p->num_objects;
> > -				}
> > -
> > -				progress = start_progress(_("Checking objects"), total);
> > +			for (p = get_all_packs(the_repository); p;
> > +			     p = p->next) {
> > +				if (open_pack_index(p))
> > +					continue;
> > +				total += p->num_objects;
> >  			}
> > +
> > +			progress = start_progress(_("Checking objects"), total,
> > +						  show_progress);
> >  			for (p = get_all_packs(the_repository); p;
> >  			     p = p->next) {
> >  				/* verify gives error messages itself */
> 
> Likewise, we do not even have to be scanning the index entries
> upfront if we are not showing the progress report (and more
> importantly, the user likely has chosen the speed/cycles over eye
> candy progress meter) while checking out paths from the index.

This was the most problematic one I saw, and I don't think it's that problematic - the loop at the bottom of the quotation above calls verify_pack(), which also calls open_pack_index(), so (unless some of the "struct packed_git" are freed in the meantime - I haven't looked at this closely) the opening of the pack indexes are not being wasted.

I also saw some strbuf manipulation to generate the title, but I also don't think that takes significant cycles compared to the task that requires the progress display.

But if this is a problem, one thing we could do is pass the total as a callback instead of as an int, and provide a generic callback that just returns the dereferenced cb_data. Most invocations would just use that generic callback. (Alternatively, as discussed in-office, we could allow start_progress() to return NULL when no progress display is needed, change start_progress() to not take a total, add a progress_set_total(), and check in display_progress() that the total has been set before proceeding.)

Show 7 quoted lines

> But the other codepaths may be doing conditional computation not
> based on "if (show_progress)" but on "if (progress)", in which case
> with this patch, we may be paying a lot more overhead even if we
> know progress meter won't be shown and the worse part from
> reviewability point of view is that this patch does not explicitly
> do anything to make it happen because start_delayed_progress() now
> unconditionally give a non-NULL progress structure to enable them.

One way to enumerate this might be to get the LHS of all the assignments from start_progress() and friends (e.g. "pi.progress" in builtin/blame.c, "progress" in builtin/commit-graph.c) and then grepping the respective files to see if "if (.*[LHS]" is done.