From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>, git@vger.kernel.org
Subject: Re: [PATCH 2/3] builtin/pack-objects.c: simplify add_objects_in_unpacked_packs()
Date: Thu, 2 Sep 2021 22:33:17 -0400 [thread overview]
Message-ID: <YTGJbYmq7O2OvqIJ@nand.local> (raw)
In-Reply-To: <YTFYhG+nK49/jR/v@coredump.intra.peff.net>
On Thu, Sep 02, 2021 at 07:04:36PM -0400, Jeff King wrote:
> On Sun, Aug 29, 2021 at 10:48:54PM -0400, Taylor Blau wrote:
>
> > +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().
>
> [...]
>
> 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.
I was trying to remember how I handled this for cruft packs (which want
to take a slightly different path to add_object_entry() in order to
record the object mtimes along the way). Indeed, in that case there is a
special add_cruft_object_entry() which uses pack and offset directly.
Having looked at all of these functions which are layered on top of
add_object_entry() recently, I tend to agree that there is some room
for clean-up there.
I plan on sending the cruft pack series soon, after I untangle all of
the `repack` + multi-pack bitmaps stuff, so perhaps that will be a good
time to pursue this.
Anyway, just to say that I probably agree that the pack mru cache is
helping us enough to make this negligible, but that it is on my mind and
we'll have a chance to look at it soon.
Thanks,
Taylor
next prev parent reply other threads:[~2021-09-03 2:33 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-30 2:48 [PATCH 0/3] pack-objects: simplify add_objects_in_unpacked_packs() Taylor Blau
2021-08-30 2:48 ` [PATCH 1/3] object-store.h: teach for_each_packed_object to ignore kept packs Taylor Blau
2021-08-30 2:48 ` [PATCH 2/3] builtin/pack-objects.c: simplify add_objects_in_unpacked_packs() Taylor Blau
2021-09-02 23:04 ` Jeff King
2021-09-03 2:33 ` Taylor Blau [this message]
2021-08-30 2:48 ` [PATCH 3/3] builtin/pack-objects.c: remove duplicate hash lookup Taylor Blau
2021-08-30 6:39 ` [PATCH 0/3] pack-objects: simplify add_objects_in_unpacked_packs() Junio C Hamano
2021-08-30 20:58 ` Jeff King
2021-08-30 21:30 ` Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YTGJbYmq7O2OvqIJ@nand.local \
--to=me@ttaylorr.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).