Re: 1.3.0 creating bigger packs than 1.2.3

[Shawn Pearce <spearce@spearce.org>]

Junio C Hamano <junkio@cox.net> wrote:
[snip]
> I suspect the test patch makes pack-objects a lot more
> expensive.

Which patch are you talking about the previous patch or the one in
the message I'm now replying to?

> The code before the test patch said "if the size is very small
> or size difference is too great, do not consider this, and do
> not consider any more objects in the delta window, because we
> know they are either even smaller of the same path, they have
> different names, or they are of different type".  The test patch
> you tried was a quick and dirty hack that said "under the
> too-small condition, skip this one, but keep trying the rest of
> the delta window".
> 
> Here is a cleaned up patch.  What it does is "under the
> too-small condition, see if the object has the same basename,
> and if so keep going, but otherwise skip the rest as before".
[snip]

The patch below does not help very much:

  Total 46391, written 46391 (delta 6686), reused 37979 (delta 0)
  129M pack-7f766f5af5547554bacb28c0294bd562589dc5e7.pack

> diff --git a/pack-objects.c b/pack-objects.c
> index 09f4f2c..2173709 100644
> --- a/pack-objects.c
> +++ b/pack-objects.c
> @@ -1036,8 +1036,6 @@ static int try_delta(struct unpacked *cu
>  	oldsize = old_entry->size;
>  	sizediff = oldsize > size ? oldsize - size : size - oldsize;
>  
> -	if (size < 50)
> -		return -1;
>  	if (old_entry->depth >= max_depth)
>  		return 0;
>  
> @@ -1048,20 +1046,27 @@ static int try_delta(struct unpacked *cu
>  	 * more space-efficient (deletes don't have to say _what_ they
>  	 * delete).
>  	 */
> -	max_size = size / 2 - 20;
> -	if (cur_entry->delta)
> -		max_size = cur_entry->delta_size-1;
> -	if (sizediff >= max_size)
> -		return -1;
> -	delta_buf = diff_delta(old->data, oldsize,
> -			       cur->data, size, &delta_size, max_size);
> -	if (!delta_buf)
> +	if (50 <= size) {
> +		max_size = size / 2 - 20;
> +		if (cur_entry->delta)
> +			max_size = cur_entry->delta_size-1;
> +		if (sizediff < max_size) {
> +			delta_buf = diff_delta(old->data, oldsize,
> +					       cur->data, size,
> +					       &delta_size, max_size);
> +			if (!delta_buf)
> +				return 0;
> +			cur_entry->delta = old_entry;
> +			cur_entry->delta_size = delta_size;
> +			cur_entry->depth = old_entry->depth + 1;
> +			free(delta_buf);
> +			return 0;
> +		}
> +	}
> +	/* Keep going as long as the basename matches */
> +	if (((cur_entry->hash ^ old_entry->hash) >>DIRBITS) == 0)
>  		return 0;
> -	cur_entry->delta = old_entry;
> -	cur_entry->delta_size = delta_size;
> -	cur_entry->depth = old_entry->depth + 1;
> -	free(delta_buf);
> -	return 0;
> +	return -1;
>  }
>  
>  static void progress_interval(int signum)
> 

-- 
Shawn.