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

From: Duy Nguyen <pclouds@gmail.com>
Date: Aug 6, 2018, 15:53 UTC
Message-ID: <CACsJy8Aa2xo+jVOPza_wt-EX8P4_M8XJD3jdLs=cr11M0ckUuA@mail.gmail.com>
In-Reply-To: <CAP8UFD2frOjmZoOfWW-93xewA6LS5zTEisNr4QDz2FNQE2DY_A@mail.gmail.com>

On Sun, Aug 5, 2018 at 8:53 PM Christian Couder <christian.couder@gmail.com> wrote:

Show 26 quoted lines

>
> 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:
>
> >> 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.

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().

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). 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.

-- 
Duy