Re: [RFC PATCH 10/10] pack-objects: improve partial packfile reuse

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2019, 22:29 UTC
Message-ID: <xmqq7e6bde4z.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190913130226.7449-11-chriscool@tuxfamily.org>

Christian Couder <christian.couder@gmail.com> writes:

> +/*
> + * Record the offsets needed in our reused packfile chunks due to
> + * "gaps" where we omitted some objects.
> + */

The meaning of 'start' and 'offset' is unclear from the first reading. Is it "starting offset" and "for how many bytes the region lasts"? If so, 'offset', which is usually a location (unless you always measure from the beginning, in which case you could say it names the byte-size of a region), may be a misnomer (side note: I'll pretend that I haven't realized the 'offset' may be the end offset of a region for now---this is a good illustration why a better comment should be here anyway ;-).

Show 8 quoted lines

> +static struct reused_chunk {
> +	off_t start;
> +	off_t offset;
> +} *reused_chunks;
> +static int reused_chunks_nr;
> +static int reused_chunks_alloc;
> +
> +static void record_reused_object(off_t where, off_t offset)

And here, 'start' is called 'where'; either is a good word for a location; we'd want to pick either one to be consistent, perhaps?

Show 6 quoted lines

>  {
> -	unsigned char buffer[8192];
> -	off_t to_write, total;
> -	int fd;
> +	if (reused_chunks_nr && reused_chunks[reused_chunks_nr-1].offset == offset)
> +		return;

The reason why the above code works is because this function will always be called in the ascending order of the 'offset'?

Hmmm, perhaps 'offset' is not a region-size after all. Is it the end offset, as opposed to 'start' which is the starting offset, and the two offsets sandwitch a region?

Show 41 quoted lines

> -	if (!is_pack_valid(reuse_packfile))
> -		die(_("packfile is invalid: %s"), reuse_packfile->pack_name);
> +	ALLOC_GROW(reused_chunks, reused_chunks_nr + 1,
> +		   reused_chunks_alloc);
> +	reused_chunks[reused_chunks_nr].start = where;
> +	reused_chunks[reused_chunks_nr].offset = offset;
> +	reused_chunks_nr++;
> +}
>  
> -	fd = git_open(reuse_packfile->pack_name);
> -	if (fd < 0)
> -		die_errno(_("unable to open packfile for reuse: %s"),
> -			  reuse_packfile->pack_name);
> +/*
> + * Binary search to find the chunk that "where" is in. Note
> + * that we're not looking for an exact match, just the first
> + * chunk that contains it (which implicitly ends at the start
> + * of the next chunk.
> + */
> +static off_t find_reused_offset(off_t where)
> +{
> +	int lo = 0, hi = reused_chunks_nr;
> +	while (lo < hi) {
> +		int mi = lo + ((hi - lo) / 2);
> +		if (where == reused_chunks[mi].start)
> +			return reused_chunks[mi].offset;
> +		if (where < reused_chunks[mi].start)
> +			hi = mi;
> +		else
> +			lo = mi + 1;
> +	}
>  
> -	if (lseek(fd, sizeof(struct pack_header), SEEK_SET) == -1)
> -		die_errno(_("unable to seek in reused packfile"));
> +	/*
> +	 * The first chunk starts at zero, so we can't have gone below
> +	 * there.
> +	 */
> +	assert(lo);
> +	return reused_chunks[lo-1].offset;
> +}

This comment has nothing to do with the change, but the way the patch is presented is quite hard to follow, in that the preimage or the common context lines do not help understand what the new code is doing at all ;-)

I'll come back to the remainder of the patch later.  Thanks.