Re: [PATCH] simple euristic for further free packing improvements

From: Junio C Hamano <junkio@cox.net>
Date: May 16, 2006, 01:51 UTC
Message-ID: <7v4pzqhh3t.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0605151129540.18071@localhost.localdomain>

Nicolas Pitre <nico@cam.org> writes:

> Given that the early eviction of objects with maximum delta depth 
> may exhibit bad packing on its own, why not considering a bias against 
> deep base objects in try_delta() to mitigate that bad behavior.

This is really a good stuff. Thanks. Oh, and thanks for noticing my puzzlement expressed with "#if 0" ;-).

Show 11 quoted lines

> @@ -1038,8 +1038,8 @@ static int try_delta(struct unpacked *tr
>  
>  	/* Now some size filtering euristics. */
>  	size = trg_entry->size;
> -	max_size = size / 2 - 20;
> -	if (trg_entry->delta)
> +	max_size = (size/2 - 20) / (src_entry->depth + 1);
> +	if (trg_entry->delta && trg_entry->delta_size <= max_size)
>  		max_size = trg_entry->delta_size-1;
>  	src_size = src_entry->size;
>  	sizediff = src_size < size ? size - src_size : 0;

At the first glance, this seems rather too agressive. It makes me wonder if it is a good balance to penalize the second generation base by requiring it to produce a small delta that is at most half as we normally would (and the third generation a third), or maybe the penalty should kick in more gradually, like e.g. ((max_depth * 2 - src_entry->depth) / (max_depth * 2).

Having said that, judging from your past patches, I learned to trust that you have tried tweaking this part and settled on this simplicity and elegance, so I'll take the patch as is -- if somebody wants to play with it that can always be done to further improve things.