Re: 2.18.0 Regression: packing performance and effectiveness

From: Jeff King <peff@peff.net>
Date: Jul 19, 2018, 17:04 UTC
Message-ID: <20180719170400.GC4868@sigill.intra.peff.net>
In-Reply-To: <20180719151640.GA24997@duynguyen.home>

On Thu, Jul 19, 2018 at 05:16:42PM +0200, Duy Nguyen wrote:

Show 9 quoted lines

> On Thu, Jul 19, 2018 at 07:57:37AM +0200, Duy Nguyen wrote:
> > I thought 2M was generous but I was obviously wrong. I'd like to see
> > the biggest delta size in this pack and whether it's still reasonable
> > to increase OE_DELTA_SIZE_BITS. But if it's very close to 4GB limit
> > then we could just store 64-bit delta size in a separate array.
> 
> I realize now that these two options don't have to be mutually
> exclusive and I should provide a good fallback in terms of performance
> anyway.

Yeah, this is what I had assumed was happening in the original code. :)

Show 13 quoted lines

> +void oe_prepare_delta_size_array(struct packing_data *pack)
> +{
> +	int i;
> +
> +	/*
> +	 * nr_alloc, not nr_objects to align with realloc() strategy in
> +	 * packlist_alloc()
> +	 */
> +	ALLOC_ARRAY(pack->delta_size, pack->nr_alloc);
> +
> +	for (i = 0; i < pack->nr_objects; i++)
> +		pack->delta_size[i] = pack->objects[i].delta_size_;
> +}

This iterator should probably be a uint32_t, to match nr_objects.

The rest of the patch looks OK to me. From Elijah's failure report there clearly is _some_ problem where we end up with a truncated write of a delta. But I don't see it from code inspection, nor could I reproduce it.

-Peff