Re: [PATCH] revision.c: reduce object database queries

From: Jeff King <peff@peff.net>
Date: Feb 26, 2018, 01:38 UTC
Message-ID: <20180226013822.GA9385@sigill.intra.peff.net>
In-Reply-To: <20180226013048.GA8677@sigill.intra.peff.net>

On Sun, Feb 25, 2018 at 08:30:48PM -0500, Jeff King wrote:

Show 25 quoted lines

> > diff --git a/revision.c b/revision.c
> > index 5ce9b93..bc7def5 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -113,7 +113,8 @@ void mark_parents_uninteresting(struct commit *commit)
> >  			 * it is popped next time around, we won't be trying
> >  			 * to parse it and get an error.
> >  			 */
> > -			if (!has_object_file(&commit->object.oid))
> > +			if (!commit->object.parsed &&
> > +			    !has_object_file(&commit->object.oid))
> >  				commit->object.parsed = 1;
> 
> We don't actually need the object contents at all right here. This is
> just faking the "parsed" flag for later so that calls to parse_object()
> don't barf.
> 
> This code comes originally form 454fbbcde3 (git-rev-list: allow missing
> objects when the parent is marked UNINTERESTING, 2005-07-10). But later,
> in aeeae1b771 (revision traversal: allow UNINTERESTING objects to be
> missing, 2009-01-27), we marked dealt with calling parse_object() on the
> parents more directly.
> 
> So what I wonder is whether this code is simply redundant and can go
> away entirely. That would save the has_object_file() call in all cases.

There's a similar case for trees. In mark_tree_contents_uninteresting() we do:

   if (!has_object_file(&obj->oid))
	return;
   if (parse_tree(tree) < 0)
	die("bad tree %s", oid_to_hex(&obj->oid));

which seems wasteful. Probably this could be:

  if (parse_tree_gently(tree, 1) < 0)
	return; /* missing uninteresting trees ok */

though technically the existing code allows _missing_ trees, but not on corrupt ones.

I guess this is perhaps less interesting, because we only mark trees directly fed from the pending array, not every tree of commits that we traverse. Though if you had a really gigantic tree, it might be measurable.

-Peff