Re: [PATCH 3/5] Add pack-objects window memory usage limit

From: Nicolas Pitre <nico@cam.org>
Date: Jul 12, 2007, 04:25 UTC
Message-ID: <alpine.LFD.0.999.0707112356330.32552@xanadu.home>
In-Reply-To: <11842100582887-git-send-email-bdowning@lavos.net>

On Wed, 11 Jul 2007, Brian Downing wrote:

Show 11 quoted lines

> +		while (window_memory_limit &&
> +		       window_memory_usage > window_memory_limit &&
> +		       count > 1) {
> +			uint32_t tail = idx - count;
> +			if (tail > idx) {
> +				tail += window + 1;
> +				tail %= window;
> +			}
> +			free_unpacked(array + tail);
> +			count--;
> +		}

This is bogus. Suppose window = 10 and only array entries 8, 9, 0, 1 and 2 are populated. In that case idx = 2 and count should be 4 (not counting the current entry yet). You want to evict entry 8.

	-- tail = 2 - 4 = -2 (or a big uint32_t value)

	-- tail > idx is true

	-- tail += window + 1 -> -2 + 10 + 1 = 9

	-- tail %= window is useless

	-- you free entry 9 instead of entry 8.

Instead, you should do:

	tail = idx - count;
	if (tail > idx)
		tail += window;

or even:

	tail = (idx + window - count) % window;

>  		next:
>  		idx++;
> +		if (count < window)
> +			count++;

And of course you want:

	if (count + 1 < window)
		count++;

So not to count the new entry when the window gets full.

Nicolas