Re: [PATCH] pack-objects: fix performance issues on packing large deltas

[Elijah Newren <newren@gmail.com>]

On Fri, Jul 20, 2018 at 10:43 AM, Elijah Newren <newren@gmail.com> wrote:
> On Fri, Jul 20, 2018 at 8:39 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

>>  If we want a quick fix for 2.18.1. I suggest bumping up
>>  OE_DELTA_SIZE_BITS like Elijah did in his second option. I don't
>>  think it's safe to fast track this patch.
>
> Okay, I'll submit that with a proper commit message then.  Since you
> also bump OE_DELTA_SIZE_BITS in your patch (though to a different
> value), it'll require a conflict resolution when merging maint into
> master, but at least the change won't silently leak through.

And here's the re-submitted patch, with commit message...

-- 8< --
Subject: [PATCH] pack-objects: fix repacking of repositories with some large
 deltas

Commit 0aca34e826 (pack-objects: shrink delta_size field in struct
object_entry - 2018-04-14) reduced 'struct object_entry' size, by dropping
the delta size field to 20 bits (max delta size 1MB).  There was a
fallback to reference existing pack files that already had large deltas,
but when aggressively repacking, it drops the deltas on the floor,
resulting in much worse packs.

For comparison, the following measurements were made for running 'git gc
--aggressive' for one particular repo:

    Version  Pack (MB)  MaxRSS(kB)  Time (s)
    -------  ---------  ----------  --------
     2.17.0     5498     43513628    2494.85
     2.18.0    10531     40449596    4168.94

This patch provides a stop-gap improvement for maint that increases the
delta size back up to 32 bits (still an improvement over the 64 bit size
of the original).  A better fix preserving the memory savings for most
repos that do not have large deltas is being prepared for 2.19.0.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
I think Duy's comment that I'm responding to suggests he Acks this
patch for maint, but there is that little 'if' he put in his statement.

I'll be on vacation until the end of the week, so I apologize in advance
for not responding quickly...

 builtin/pack-objects.c | 2 +-
 pack-objects.h         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 71056d8294..49b7af295d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2023,7 +2023,7 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 	delta_buf = create_delta(src->index, trg->data, trg_size, &delta_size, max_size);
 	if (!delta_buf)
 		return 0;
-	if (delta_size >= (1U << OE_DELTA_SIZE_BITS)) {
+	if (delta_size >= (1UL << OE_DELTA_SIZE_BITS)) {
 		free(delta_buf);
 		return 0;
 	}
diff --git a/pack-objects.h b/pack-objects.h
index edf74dabdd..26021328aa 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -14,7 +14,7 @@
  * above this limit. Don't lower it too much.
  */
 #define OE_SIZE_BITS		31
-#define OE_DELTA_SIZE_BITS	20
+#define OE_DELTA_SIZE_BITS	32
 
 /*
  * State flags for depth-first search used for analyzing delta cycles.
-- 
2.18.0.2.gf4c50c7885