Re: [PATCH] pack-objects: fix performance issues on packing large deltas
- From
Elijah Newren <newren@gmail.com>
- Date
- Jul 23, 2018, 12:34 UTC
- Message-ID
- <20180723123423.32186-1-newren@gmail.com>
- In-Reply-To
- <CABPp-BGJAWXOCPsej=fWWDJkh-7BAV9m8yEDiy2NVkGTRCmk4A@mail.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:
Show 8 quoted lines
>> 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.94This 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