On Thu, Jan 11, 2018 at 07:57:42PM +0100, René Scharfe wrote: > > 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. > > 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
