Re: [PATCH v4 08/11] pack-objects: shrink z_delta_size field in struct object_entry

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2018, 19:40 UTC
Message-ID: <xmqqr2oj24j1.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180316183200.31014-9-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 7 quoted lines

> We only cache deltas when it's smaller than a certain limit. This limit
> defaults to 1000 but save its compressed length in a 64-bit field.
> Shrink that field down to 16 bits, so you can only cache 65kb deltas.
> Larger deltas must be recomputed at when the pack is written down.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Show 12 quoted lines

>  		if (entry->delta_data && !pack_to_stdout) {
> -			entry->z_delta_size = do_compress(&entry->delta_data,
> -							  entry->delta_size);
> -			cache_lock();
> -			delta_cache_size -= entry->delta_size;
> -			delta_cache_size += entry->z_delta_size;
> -			cache_unlock();
> +			unsigned long size;
> +
> +			size = do_compress(&entry->delta_data, entry->delta_size);
> +			entry->z_delta_size = size;
> +			if (entry->z_delta_size == size) {

It is confusing to readers to write

	A = B;
	if (A == B) {
		/* OK, A was big enough */
	} else {
		/* No, B is too big to fit on A */
	}

I actually was about to complain that you attempted an unrelated micro-optimization to skip cache_lock/unlock when delta_size and z_delta_size are the same, and made a typo. Something like:

	size = do_compress(...);
	if (size < (1 << OE_Z_DELTA_BITS)) {
		entry->z_delta_size = size;
		cache_lock();
		...
                cache_unlock();
	} else {
		FREE_AND_NULL(entry->delta_data);
		entry->z_delta_size = 0;
	}

would have saved me a few dozens of seconds of head-scratching.

Show 9 quoted lines

> +				cache_lock();
> +				delta_cache_size -= entry->delta_size;
> +				delta_cache_size += entry->z_delta_size;
> +				cache_unlock();
> +			} else {
> +				FREE_AND_NULL(entry->delta_data);
> +				entry->z_delta_size = 0;
> +			}
>  		}