Re: [PATCH 2/3] builtin/pack-objects.c: simplify add_objects_in_unpacked_packs()
- From
Jeff King <peff@peff.net>
- Date
- Sep 2, 2021, 23:04 UTC
- Message-ID
- <YTFYhG+nK49/jR/v@coredump.intra.peff.net>
- In-Reply-To
- <c857e12a032f197626cd6a5eb0eafc66afbb5bed.1630291682.git.me@ttaylorr.com>
On Sun, Aug 29, 2021 at 10:48:54PM -0400, Taylor Blau wrote:
Show 13 quoted lines
> +static int add_object_in_unpacked_pack(const struct object_id *oid,
> + struct packed_git *pack,
> + uint32_t pos,
> + void *_data)
> {
> [...]
> + struct object *obj = lookup_unknown_object(the_repository, oid);
> + if (obj->flags & OBJECT_ADDED)
> + return 0;
> + add_object_entry(oid, obj->type, "", 0);
> + obj->flags |= OBJECT_ADDED;
> + return 0;
> }This is not new in your patch series, but while merging this with another topic I had, I noticed another opportunity for optimization here. We already know the pack/pos in which we found the object. But when we call add_object_entry(), it will do another lookup, via want_object_in_pack().
We could shortcut that by providing it with the extra information, the way add_object_entry_from_bitmap() does. The original code before your series could have done the same optimization, but it became much more obvious after your series, since -Wunused-parameters notices that we do not look at the "pack" or "pos" parameters at all. :)
It may not be that exciting an optimization, though. Pack lookups aren't _that_ expensive, and the pack-mru code would mean we always find it in the first pack (since by definition we're iterating through the objects in whole packs, our locality is perfect).
It would also probably involve some slight refactoring of add_object_entry() to avoid duplication (though possibly the result could reduce similar duplication with the bitmap variant). Hmm, actually looking further, we already have add_object_entry_from_pack() for --stdin-packs.
So I offer it mainly as an observation, in case somebody wants to look into it further (both for the optimization and the possibility of simplifying the code).
-Peff