Re: [PATCH v2 4/9] object: add clear_commit_marks_all()

From: Jeff King <peff@peff.net>
Date: Jan 12, 2018, 15:20 UTC
Message-ID: <20180112152058.GA10210@sigill.intra.peff.net>
In-Reply-To: <a0ca86b8-e258-2588-1c99-a30e8e60fdbd@web.de>

On Thu, Jan 11, 2018 at 07:57:42PM +0100, René Scharfe wrote:

Show 13 quoted lines

> > Is it worth having:
> > 
> >    void clear_object_flags_from_type(int type, unsigned flags);
> > 
> > rather than having two near-identical functions? I guess we'd need some
> > way of saying "all types" to reimplement clear_object_flags() as a
> > wrapper (OBJ_NONE, I guess?).
> 
> I don't know if there is a demand.  Perhaps the two callers of
> clear_object_flags() should be switched to clear_commit_marks_all()?
> They look like they only care about commits as well.  Or is it safe to
> stomp over the flags of objects of other types?  Then we'd only need
> to keep clear_object_flags()..

I'd worry that the call in reset_revision_walk() might want to clear non-commits if the revisions have "--objects" passed to them.

I do suspect that clearing flags from all objects would just work in the general case (since we're limiting ourselves to only a particular set of flags). But it's probably not worth the risk of unintended fallout, since there's not much benefit after your series.

Show 15 quoted lines

> > The run-time check is maybe a little bit slower in the middle of a tight
> > loop, but I'm not sure it would matter much (I'd actually be curious if
> > this approach is faster than the existing traversal code, too).
> 
> I don't know how to measure this properly.  With 100 runs each I get
> this for the git repo and the silly test program below, which measures
> the duration of the respective function call:
> 
>    mean        stddev method
>    ----------- ------ ----------------------
>    5.89763e+06 613106 clear_commit_marks
>    2.72572e+06 507689 clear_commit_marks_all
>    1.96582e+06 494753 clear_object_flags
> 
> So these are noisy numbers, but kind of in the expected range.

That's about what I'd expect. The "bad" case for looking at all objects is when there are a bunch of objects loaded that _weren't_ part of this particular traversal. I have no idea how often that happens, but we can guess at the impact in the worst case: having done a previous --objects traversal in the process and then traversing all of the commits a second time, we'd probably have about 5-10x as many objects to look through for that second path. So clear_commit_marks() would win there.

The absolute numbers are small enough that it probably doesn't matter either way.

-Peff