Re: [RFC PATCH 2/5] Add delta-islands.{c,h}

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 5, 2018, 18:53 UTC
Message-ID: <CAP8UFD2frOjmZoOfWW-93xewA6LS5zTEisNr4QDz2FNQE2DY_A@mail.gmail.com>
In-Reply-To: <CACsJy8AzksB=rfaX_dfboMXHjqj6gj+erdF6eRFAKmWA1-3PUg@mail.gmail.com>

On Sun, Jul 22, 2018 at 10:50 AM, Duy Nguyen <pclouds@gmail.com> wrote:

> On Sun, Jul 22, 2018 at 7:51 AM Christian Couder
> <christian.couder@gmail.com> wrote:

Show 16 quoted lines

>> diff --git a/pack-objects.h b/pack-objects.h
>> index edf74dabdd..8eecd67991 100644
>> --- a/pack-objects.h
>> +++ b/pack-objects.h
>> @@ -100,6 +100,10 @@ struct object_entry {
>>         unsigned type_:TYPE_BITS;
>>         unsigned no_try_delta:1;
>>         unsigned in_pack_type:TYPE_BITS; /* could be delta */
>> +
>> +       unsigned int tree_depth; /* should be repositioned for packing? */
>> +       unsigned char layer;
>> +
>
> This looks very much like an optional feature. To avoid increasing
> pack-objects memory usage for common case, please move this to struct
> packing_data.

As you can see the patch 6/6 (in the v2 of this patch series that I just sent) moves `unsigned int tree_depth` from 'struct object_entry' to 'struct packing_data'. I am not sure that I did it right and that it is worth it though as it is a bit more complex.

About `unsigned char layer` I am even less sure that it's worth it for the following reasons:

- 'struct object_entry' contains bit fields as its last members and
then the following comment:

    /*
     * pahole results on 64-bit linux (gcc and clang)
     *
     *   size: 80, bit_padding: 20 bits, holes: 8 bits
     *
     * and on 32-bit (gcc)
     *
     *   size: 76, bit_padding: 20 bits, holes: 8 bits
     */

I am not sure if this comment is still up to date, but if it true maybe there is enough space in the padding to add 'layer' as a 8 bit bit field somewhere without increasing the size of 'struct object_entry'.

- 'layer' is used in add_to_write_order() which is called from many
places in add_descendants_to_write_order() and compute_write_order()
for example like this:

            for (s = DELTA_SIBLING(e); s; s = DELTA_SIBLING(s)) {
                add_to_write_order(wo, endp, s);
            }

So it seems to me that moving 'layer' to 'struct packing_data' would require changing the DELTA_SIBLING macros or adding a hack to easily find the index in an array corresponding to a 'struct object_entry'.

- I don't see why it is different from other fields in 'struct
object_entry' that are also used in compute_write_order(), for
example:

    uint32_t delta_idx;    /* delta base object */
    uint32_t delta_child_idx; /* deltified objects who bases me */
    uint32_t delta_sibling_idx; /* other deltified objects who ... */

Would we also want to move these fields to 'struct packing_data'? Why or why not? If we want to move them, then it might be a good idea to do all the moving at the same time to make sure we get something coherent.

Thanks, Christian.