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

From: Jonathan Tan <jonathantanmy@google.com>
To: gitster@pobox.com
Cc: emilyshaffer@google.com, git@vger.kernel.org,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode
Date: Wed,  9 Sep 2020 15:36:31 -0700	[thread overview]
Message-ID: <20200909223631.864145-1-jonathantanmy@google.com> (raw)
In-Reply-To: <xmqq365zro2u.fsf@gitster.c.googlers.com>

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

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