[1/2] commit-graph: don't show progress percentages while expanding reachable commits
diff mbox series

Message ID 20190907050132.GA23904@sigill.intra.peff.net
State New
Headers show
Series
  • a few commit-graph improvements
Related show

Commit Message

Jeff King Sept. 7, 2019, 5:01 a.m. UTC
From: SZEDER Gábor <szeder.dev@gmail.com>

Commit 49bbc57a57 (commit-graph write: emit a percentage for all
progress, 2019-01-19) was a bit overeager when it added progress
percentages to the "Expanding reachable commits in commit graph" phase
as well, because most of the time the number of commits that phase has
to iterate over is not known in advance and grows significantly, and,
consequently, we end up with nonsensical numbers:

  $ git commit-graph write --reachable
  Expanding reachable commits in commit graph: 138606% (824706/595), done.
  [...]

  $ git rev-parse v5.0 | git commit-graph write --stdin-commits
  Expanding reachable commits in commit graph: 81264400% (812644/1), done.
  [...]

Even worse, because the percentage grows so quickly, the progress code
outputs much more often than it should (because it ticks every second,
or every 1%), slowing the whole process down. My time for "git
commit-graph write --reachable" on linux.git went from 13.463s to
12.521s with this patch, ~7% savings.

Therefore, don't show progress percentages in the "Expanding reachable
commits in commit graph" phase.

Note that the current code does sometimes do the right thing, if we
picked up all commits initially (e.g., omitting "--reachable" in a
fully-packed repository would get the correct count without any parent
traversal). So it may be possible to come up with a way to tell when we
could use a percentage here. But in the meantime, let's make sure we
robustly avoid printing nonsense.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Compared to the original from:

  https://public-inbox.org/git/20190322102817.19708-1-szeder.dev@gmail.com/

I rebased it to handle code movement, added in the timing data, and
tried to summarize the discussion from the thread.

 commit-graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

SZEDER Gábor Sept. 7, 2019, 10:34 a.m. UTC | #1
On Sat, Sep 07, 2019 at 01:01:33AM -0400, Jeff King wrote:
> From: SZEDER Gábor <szeder.dev@gmail.com>
> 
> Commit 49bbc57a57 (commit-graph write: emit a percentage for all
> progress, 2019-01-19) was a bit overeager when it added progress
> percentages to the "Expanding reachable commits in commit graph" phase
> as well, because most of the time the number of commits that phase has
> to iterate over is not known in advance and grows significantly, and,
> consequently, we end up with nonsensical numbers:
> 
>   $ git commit-graph write --reachable
>   Expanding reachable commits in commit graph: 138606% (824706/595), done.
>   [...]
> 
>   $ git rev-parse v5.0 | git commit-graph write --stdin-commits
>   Expanding reachable commits in commit graph: 81264400% (812644/1), done.
>   [...]
> 
> Even worse, because the percentage grows so quickly, the progress code
> outputs much more often than it should (because it ticks every second,
> or every 1%), slowing the whole process down. My time for "git
> commit-graph write --reachable" on linux.git went from 13.463s to
> 12.521s with this patch, ~7% savings.

Oh, interesting.

> Therefore, don't show progress percentages in the "Expanding reachable
> commits in commit graph" phase.
> 
> Note that the current code does sometimes do the right thing, if we
> picked up all commits initially (e.g., omitting "--reachable" in a
> fully-packed repository would get the correct count without any parent
> traversal). So it may be possible to come up with a way to tell when we
> could use a percentage here. But in the meantime, let's make sure we
> robustly avoid printing nonsense.
> 
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Compared to the original from:
> 
>   https://public-inbox.org/git/20190322102817.19708-1-szeder.dev@gmail.com/
> 
> I rebased it to handle code movement, added in the timing data, and
> tried to summarize the discussion from the thread.

Thanks for resurrecting this patch and for the summary paragraph.
Taylor Blau Sept. 7, 2019, 6:54 p.m. UTC | #2
On Sat, Sep 07, 2019 at 12:34:07PM +0200, SZEDER Gábor wrote:
> On Sat, Sep 07, 2019 at 01:01:33AM -0400, Jeff King wrote:
> > From: SZEDER Gábor <szeder.dev@gmail.com>
> >
> > Commit 49bbc57a57 (commit-graph write: emit a percentage for all
> > progress, 2019-01-19) was a bit overeager when it added progress
> > percentages to the "Expanding reachable commits in commit graph" phase
> > as well, because most of the time the number of commits that phase has
> > to iterate over is not known in advance and grows significantly, and,
> > consequently, we end up with nonsensical numbers:
> >
> >   $ git commit-graph write --reachable
> >   Expanding reachable commits in commit graph: 138606% (824706/595), done.
> >   [...]
> >
> >   $ git rev-parse v5.0 | git commit-graph write --stdin-commits
> >   Expanding reachable commits in commit graph: 81264400% (812644/1), done.
> >   [...]
> >
> > Even worse, because the percentage grows so quickly, the progress code
> > outputs much more often than it should (because it ticks every second,
> > or every 1%), slowing the whole process down. My time for "git
> > commit-graph write --reachable" on linux.git went from 13.463s to
> > 12.521s with this patch, ~7% savings.
>
> Oh, interesting.
>
> > Therefore, don't show progress percentages in the "Expanding reachable
> > commits in commit graph" phase.
> >
> > Note that the current code does sometimes do the right thing, if we
> > picked up all commits initially (e.g., omitting "--reachable" in a
> > fully-packed repository would get the correct count without any parent
> > traversal). So it may be possible to come up with a way to tell when we
> > could use a percentage here. But in the meantime, let's make sure we
> > robustly avoid printing nonsense.
> >
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > Compared to the original from:
> >
> >   https://public-inbox.org/git/20190322102817.19708-1-szeder.dev@gmail.com/
> >
> > I rebased it to handle code movement, added in the timing data, and
> > tried to summarize the discussion from the thread.
>
> Thanks for resurrecting this patch and for the summary paragraph.

Thanks from me, as well. I noticed that we had achieved three billion
percent progress on the repository that brought this to our attention,
but didn't notice that you had already written these patches.

So, I am glad that they are getting the attribution that they deserve.
Thanks again both.

Thanks,
Taylor
Linus Torvalds Sept. 27, 2019, 6:54 p.m. UTC | #3
I was going to make a bug-report about this funny behavior, but
decided to search the list first.

Yeah, I smiled at

    Expanding reachable commits in commit graph: 139276% (870481/625), done.

when I did the "git gc --prune=now" on the kernel, and apparently
actually looked at the noise for the first time in a while.

So I don't mind the bug, but I think this should be fixed. Because
almost 140 _thousand_ percent just isn't right.

So Ack on this patch. Looks like it is already in 'pu', but hasn't
made it into next or master yet.

                    Linus

On Fri, Sep 6, 2019 at 10:04 PM Jeff King <peff@peff.net> wrote:
>
> From: SZEDER Gábor <szeder.dev@gmail.com>
>
> Commit 49bbc57a57 (commit-graph write: emit a percentage for all
> progress, 2019-01-19) was a bit overeager when it added progress
> percentages/ [...]

Patch
diff mbox series

diff --git a/commit-graph.c b/commit-graph.c
index f2888c203b..d6a5c8cf1c 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1050,7 +1050,7 @@  static void close_reachable(struct write_commit_graph_context *ctx)
 	if (ctx->report_progress)
 		ctx->progress = start_delayed_progress(
 					_("Expanding reachable commits in commit graph"),
-					ctx->oids.nr);
+					0);
 	for (i = 0; i < ctx->oids.nr; i++) {
 		display_progress(ctx->progress, i + 1);
 		commit = lookup_commit(ctx->r, &ctx->oids.list[i]);