Re: [PATCH 2/9] fsck_tree(): fix shadowed variable

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 4, 2021, 10:10 UTC
Message-ID: <87fsz2wupo.fsf@evledraar.gmail.com>
In-Reply-To: <YJBZYt8ivlTDHpRM@coredump.intra.peff.net>

On Mon, May 03 2021, Jeff King wrote:

Show 25 quoted lines

> On Mon, May 03, 2021 at 01:15:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > Commit b2f2039c2b (fsck: accept an oid instead of a "struct tree" for
>> > fsck_tree(), 2019-10-18) introduced a new "oid" parameter to
>> > fsck_tree(), and we pass it to the report() function when we find
>> [...]
>> 
>> Have you considered just passing down the "obj" instead of the "oid"?
>> It's a bit of a bigger change, but seems to be worth it in this case as
>> the below diff (on top of master) shows. We spend quite a bit of effort
>> peeling an "obj" just to pass oid/type further down the stack.
>
> That would be undoing the point of the referenced commit above. More
> discussion in this thread:
>
>   https://lore.kernel.org/git/20191018044103.GA17625@sigill.intra.peff.net/
>
> but the gist is:
>
>   - there were bugs caused by looking at the object structs; not having
>     it make sure we can't introduce similar bugs
>
>   - using oids and buffers makes it possible for code to avoid having
>     object structs at all. index-pack could take advantage of this (to
>     reduce memory usage), but nobody has yet written that patch.

Ah, I'd managed to read this series + done some previous fsck hacking without noticing that recent-ish change.

FWIW I'd been meaning to slowly mutate some of the fsck code somewhat in the opposite direction. I.e. we have N object parsers now between {blob,commit,tree,tag,fsck,tree-walk}.c and elsewhere. Ideally we could share all/most of the logic and piggy-back in fsck on the "main" parser in some more exhaustive mode.

Unfortunately most of our fsck tests are rather non-exhaustive, so I see e.g. in reverting some of that on our current test suite e.g. a test in directory.t6102-rev-list-unexpected-objects was changed from:

    error in commit 536c6159e861ef1ac7967a68324a8e7614632970: missingParent: parent objects missing

To:

    error in commit 536c6159e861ef1ac7967a68324a8e7614632970: broken links

So by using the "main" parser in this case we lost the ability to selectively ignore that error, looks like nobody cared (and the OID could still be added to the skiplist).

Another thing that got worse (but we arguably never supported well) is fsck_commit() and expecting to validate at least a single commit object. Before we'd notice missing parents AFAICT, after we don't at that entry point, but will of course if entering via fsck_walk().

I don't think any current caller cares, but when I rewrote builtin/mktag.c recently there was a suggestion to do that "fsck the new object" for the low-level commit/tree/blob APIs too. Before (and again, AFACT) that would work to a level of 1, after you'll need to instrument some limited walk.

Anyway. This inspired me to try re-arranging the "struct object" to be a superset of "struct object_id", i.e. to make it start with the oid instead of "parsed" etc. It means you can do this:

	static inline struct object_id *object_to_oid(const struct object *obj)
	{
	        return (struct object_id *)obj;
	}

And change common cases like oid_to_hex(&obj->oid) to oid_to_hex(object_to_oid(obj)), i.e. a wrapper for a pure cast. Just that seems to speed up fsck by 1-3%, but I'm not 100% certain yet. The cachegrind numbers look unambiguously better. This is with gcc and -O3.

More generally we have various other structures that wrap an "oid" in one way or the other, where the common access pattern is just grabbing the OID out of it, but it's not the first entry. E.g. "name_entry", "directory" etc. By re-arranging it like that we could also have a "object_id_type" which is a 2-member touple of "oid/type", so by having that "oid/type" first in "struct object" you can cast "struct object" to "struct object_id_type" for fsck.c use, and further cast a "struct object_id_type" to "struct object_id" for anything that needs that.

Anyway, right now I don't have anything conclusive. Just thought I'd poke you/others with the above in case it's an appraoch that's been tried/considered before.