Re: 2.18.0 Regression: packing performance and effectiveness

From: Duy Nguyen <pclouds@gmail.com>
Date: Jul 19, 2018, 17:31 UTC
Message-ID: <20180719173135.GA24563@duynguyen.home>
In-Reply-To: <20180719172358.GD4868@sigill.intra.peff.net>

On Thu, Jul 19, 2018 at 01:23:58PM -0400, Jeff King wrote:

Show 23 quoted lines

> On Thu, Jul 19, 2018 at 09:42:00AM -0700, Elijah Newren wrote:
> 
> > Thanks for the quick turnaround.  Unfortunately, I have some bad news.
> > With this patch, I get the following:
> > 
> > $ /usr/bin/time -f 'MaxRSS:%M Time:%e' git gc --aggressive
> > Enumerating objects: 4460703, done.
> > Counting objects: 100% (4460703/4460703), done.
> > Delta compression using up to 40 threads.
> > Compressing objects: 100% (3807140/3807140), done.
> > Writing objects: 100% (4460703/4460703), done.
> > Total 4460703 (delta 2831383), reused 1587071 (delta 0)
> > error: failed to unpack compressed delta at offset 183854150 from
> > .git/objects/pack/pack-30d4f0b0e5a03dc91a658a0586f4e74cdf4a94d6.pack
> > fatal: packed object 20ce811e53dabbb8ef9368c108cbbdfa65639c03 (stored
> > in .git/objects/pack/pack-30d4f0b0e5a03dc91a658a0586f4e74cdf4a94d6.pack)
> > is corrupt
> > error: failed to run prune
> > MaxRSS:40025196 Time:2531.52
> 
> Looking at that output, my _guess_ is that we somehow end up with a
> bogus delta_size value and write out a truncated entry. But I couldn't
> reproduce the issue with smaller test cases.

Could it be a race condition? I ran the whole test suite with this fallback code activated (by forcing the delta size limit down to two bytes) and nothing failed. Perhaps something like this on top? I'll need to see if helgrind could spot anything...

-- 8< --

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 7f3546057d..1eccbc91d2 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2054,7 +2054,16 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 	}
 
 	SET_DELTA(trg_entry, src_entry);
+
+	/*
+	 * Locking is needed because SET_DELTA_SIZE() internally call
+	 * oe_prepare_delta_size_array() which may touch other entries,
+	 * which are updated in parallel.
+	 */
+	cache_lock();
 	SET_DELTA_SIZE(trg_entry, delta_size);
+	cache_unlock();
+
 	trg->depth = src->depth + 1;
 
 	return 1;
diff --git a/pack-objects.c b/pack-objects.c
index 89699cf15c..9e52af32c3 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -185,13 +185,16 @@ struct object_entry *packlist_alloc(struct packing_data *pdata,
 void oe_prepare_delta_size_array(struct packing_data *pack)
 {
 	uint32_t i;
+	uint32_t *delta_size;
 
 	/*
 	 * nr_alloc, not nr_objects to align with realloc() strategy in
 	 * packlist_alloc()
 	 */
-	ALLOC_ARRAY(pack->delta_size, pack->nr_alloc);
+	ALLOC_ARRAY(delta_size, pack->nr_alloc);
 
 	for (i = 0; i < pack->nr_objects; i++)
-		pack->delta_size[i] = pack->objects[i].delta_size_;
+		delta_size[i] = pack->objects[i].delta_size_;
+
+	pack->delta_size = delta_size;
 }
-- 8< --

Elijah, another thing you could try (if you have plenty of time to
spare) is run this repack with a single thread. It's going to take
forever though...

--
Duy