Re: [PATCH v3 4/6] pack-bitmap: prepare to read lookup table extension
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jul 15, 2022, 22:20 UTC
- Message-ID
- <YtHoJ90N6rmDmn6M@nand.local>
- In-Reply-To
- <CAPOJW5y+ywbiT2XBYYNN+y73+V98Ro33D1bgZQveQLTPfrgE_g@mail.gmail.com>
On Fri, Jul 15, 2022 at 10:08:17PM +0530, Abhradeep Chakraborty wrote:
Show 27 quoted lines
> On Fri, Jul 15, 2022 at 8:16 AM Taylor Blau <me@ttaylorr.com> wrote:
> >
> > On Mon, Jul 04, 2022 at 08:46:14AM +0000, Abhradeep Chakraborty via GitGitGadget wrote:
> > > +/*
> > > + * Searches for a matching triplet. `va` is a pointer
> > > + * to the wanted commit position value. `vb` points to
> > > + * a triplet in lookup table. The first 4 bytes of each
> > > + * triplet (pointed by `vb`) are compared with `*va`.
> > > + */
> > > +static int triplet_cmp(const void *va, const void *vb)
> > > +{
> > > +
> > > + uint32_t a = *(uint32_t *)va;
> >
> > The comment you added is definitely helpful, but I still think that this
> > line is a little magical. `*va` isn't really a pointer to a `uint32_t`,
> > but a pointer to the start of a triplet, which just *happens* to have a
> > 4-byte integer at the beginning of it.
>
> Are you sure about this? As far as I know, the first parameter of such
> comparing functions is always a pointer to the given key that we need
> to search for and the second parameter points to each element of an
> array.
>
> I think "`va is a pointer to the wanted commit position value" is not
> that descriptive. Maybe "`va` is a pointer to the given key" is
> better. What do you think?Yes, the first argument to the comparison function used in bsearch() is a pointer to some element in the array. I just meant that that array is the bitmap_git->table_lookup region, so each element isn't actually a uint32_t array, but the whole thing is an array of (uint32_t, uint64_t, uint32_t) triplets.
What you wrote here is fine, and I don't even think that the comment needs updating. If you did want to clarify, I think you could say something along the lines of what you wrote above ("`va` is a pointer to an array element") and add something along the lines of "where the array is the lookup table region of the .bitmap".
Show 27 quoted lines
> > > + ALLOC_ARRAY(xor_items, xor_items_alloc);
> >
> > This ALLOC_ARRAY() looks great to me. I wonder if we could amortize the
> > cost of allocating in this (somewhat) hot function by treating the
> > `xor_items` array as a reusable static buffer where we reset
> > xor_items_nr to 0 when entering this function.
> >
> > > + while (xor_row != 0xffffffff) {
> > > + struct object_id xor_oid;
> > > +
> > > + if (xor_items_nr + 1 >= bitmap_git->entry_count) {
> > > + free(xor_items);
> > > + error(_("corrupt bitmap lookup table: xor chain exceed entry count"));
> >
> > I think we can probably `die()` here, we're pretty much out of luck in
> > this case.
> > ...
> > > + error(_("corrupt bitmap lookup table: commit index %u out of range"),
> > > + triplet.commit_pos);
> >
> > Same here.
>
> I didn't use `die()` here because I thought returning NULL would be a
> better idea. In that case, Git can still do its job by using the
> traditional approach - traversing between objects.
> `load_bitmap_entries_v1` also returns NULL if an error occurs. What do
> you think?Ah, I wasn't aware that our callers are graceful enough to handle this like that. Yes, if we can fallback gracefully, we should, so I think just error()-ing here (and above) is the right choice. Thanks for saying so.
Thanks, Taylor