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

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 6, 2018, 18:54 UTC
Message-ID: <CAP8UFD35j1KkO=hB58Qcs=PZTFW2=djGi07z5uyayDENXzanwA@mail.gmail.com>
In-Reply-To: <CACsJy8Aa2xo+jVOPza_wt-EX8P4_M8XJD3jdLs=cr11M0ckUuA@mail.gmail.com>

On Mon, Aug 6, 2018 at 5:53 PM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 13 quoted lines

> On Sun, Aug 5, 2018 at 8:53 PM Christian Couder
> <christian.couder@gmail.com> wrote:
>>
>> 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.
>
> It is a bit more complex than I expected. But I think if you go with
> Jeff's suggestion (i.e. think of the new tree_depth array an extension
> of objects array) it's a bit simpler: you access both arrays using the
> same index, both arrays should have the same size and be realloc'd at
> the same time in packlist_alloc().

Ok, I will take a look at doing that to simplify things. Thanks to Peff and you for that suggestion!

> Is it worth it? The "pahole" comment in this file is up to date. We
> use 80 bytes per object. This series makes the struct 88 bytes (I've
> just rerun pahole).

Did you run it on V1 or on V2? I guess on V2, but then what do you think about converting the 'layer' field into a bit field, which might be simpler and save space?

> On linux repo with 12M objects, "git pack-objects
> --all" needs extra 96MB memory even if this feature is not used. So
> yes I still think it's worth moving these fields out of struct
> object_entry.

And what about the fields already in struct object_entry? While I am at it, I think I could move some of them too if it is really so worth it.