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

On Sun, Aug 5, 2018 at 8:53 PM Christian Couder
[off-list ref] wrote:

On Sun, Jul 22, 2018 at 10:50 AM, Duy Nguyen [off-list ref] wrote:

quoted

On Sun, Jul 22, 2018 at 7:51 AM Christian Couder
[off-list ref] wrote:

quoted

quoted

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