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.
next prev parent reply other threads:[~2020-09-09 22:37 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-10 1:42 [PATCH 0/2] enable progress traces even when quiet Emily Shaffer
2020-07-10 1:42 ` [PATCH 1/2] progress: create progress struct in 'verbose' mode Emily Shaffer
2020-07-10 2:00 ` Derrick Stolee
2020-07-10 2:17 ` Taylor Blau
2020-07-10 19:21 ` Emily Shaffer
2020-07-10 2:14 ` brian m. carlson
2020-07-10 19:24 ` Emily Shaffer
2020-07-10 21:09 ` Junio C Hamano
2020-07-10 22:00 ` Emily Shaffer
2020-07-10 22:40 ` Junio C Hamano
2020-07-14 0:15 ` Emily Shaffer
2020-08-17 22:19 ` Emily Shaffer
2020-08-17 22:44 ` Junio C Hamano
2020-08-17 22:49 ` Junio C Hamano
2020-09-09 22:42 ` Jonathan Tan
2020-09-09 22:36 ` Jonathan Tan [this message]
2020-09-09 23:06 ` Junio C Hamano
2020-09-10 0:31 ` Jonathan Nieder
2020-09-10 5:09 ` Junio C Hamano
2020-07-10 1:42 ` [PATCH 2/2] progress: remove redundant null-checking Emily Shaffer
2020-07-10 2:01 ` Derrick Stolee
2020-07-10 2:20 ` Taylor Blau
2020-07-10 18:50 ` Junio C Hamano
2020-07-10 19:27 ` Emily Shaffer
2020-07-10 19:58 ` Junio C Hamano
2020-07-10 20:29 ` Emily Shaffer
2020-07-10 23:03 ` Emily Shaffer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200909223631.864145-1-jonathantanmy@google.com \
--to=jonathantanmy@google.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).