Re: [PATCH 4/7] for_each_packed_object: support iterating in pack-order
- From
Jeff King <peff@peff.net>
- Date
- Aug 16, 2018, 17:36 UTC
- Message-ID
- <20180816173639.GB882@sigill.intra.peff.net>
- In-Reply-To
- <2ac79ea0-dd58-965d-1158-d08a7731e25d@gmail.com>
On Wed, Aug 15, 2018 at 09:28:44AM -0400, Derrick Stolee wrote:
Show 21 quoted lines
> On 8/10/2018 7:15 PM, Jeff King wrote:
> > diff --git a/commit-graph.c b/commit-graph.c
> > index b0a55ad128..69a0d1c203 100644
> > --- a/commit-graph.c
> > +++ b/commit-graph.c
> > @@ -730,7 +730,7 @@ void write_commit_graph(const char *obj_dir,
> > die("error adding pack %s", packname.buf);
> > if (open_pack_index(p))
> > die("error opening index for %s", packname.buf);
> > - for_each_object_in_pack(p, add_packed_commits, &oids);
> > + for_each_object_in_pack(p, add_packed_commits, &oids, 0);
> > close_pack(p);
> > }
>
> This use in write_commit_graph() is actually a good candidate for
> pack-order, since we are checking each object to see if it is a commit. This
> is only used when running `git commit-graph write --stdin-packs`, which is
> how VFS for Git maintains the commit-graph.
>
> I have a note to run performance tests on this case and follow up with a
> change on top of this series that adds the FOR_EACH_OBJECT_PACK_ORDER flag.I doubt that it will show the dramatic improvement in CPU that I mentioned in my commit message, because most of that comes from more efficient use of the delta cache. But it's very rare for commits to be deltas (usually it's just almost-twins due to cherry-picks and rebases).
So you may benefit from block cache efficiency on a cold-cache or on a system under memory pressure, but I wouldn't expect much change at all for the warm-cache case.
I doubt it will hurt, though; you'll pay for the revindex generation, but that's probably not a big deal compared to walking all the objects.
One thing you _could_ do is stop walking through the pack when you see a non-commit, since we stick all of the commits at the front. But that's just what the code happens to do, and not a strict promise. So I think it's a bad idea to rely on it (and in fact the delta-islands work under discussion elsewhere will break that assumption).
-Peff