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