Re: [PATCH 2/2] log: do not shorten decoration names too early

From: Jeff King <peff@peff.net>
Date: May 14, 2015, 21:54 UTC
Message-ID: <20150514215453.GA16897@peff.net>
In-Reply-To: <xmqqfv6yj0dl.fsf@gitster.dls.corp.google.com>

On Thu, May 14, 2015 at 02:49:10PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> > But if we do not have the "buffer" data cached for that commit (via
> > an earlier call to set_commit_buffer()), we don't have to enlarge
> > the slab, as we are not adding anything to the slab system with this
> > call.
> 
> The change may look something like this.  I do not think it would
> make a difference to the get_cached_commit_buffer() codepath (when
> we use the commit->buffer, we pretty much know we use that for all
> commits), though.

I'm not sure that's true. Most of the _users_ of the commit buffer will try to look in the cache, and if it's not there, do a one-off read. But they don't attach the result to the commit; they throw it away. The reasoning is that we don't have a cached buffer because we are going to look at a lot of commits (i.e., save_commit_buffer is off).

So basically anytime save_commit_buffer is off (e.g., in rev-list) we are expanding the slab unnecessarily, even though literally nobody will write to it.

Show 18 quoted lines

> diff --git a/commit.c b/commit.c
> index 65179f9..2d1901f 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -244,7 +244,12 @@ void set_commit_buffer(struct commit *commit, void *buffer, unsigned long size)
>  
>  const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)
>  {
> -	struct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);
> +	struct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);
> +	if (!v) {
> +		if (sizep)
> +			*sizep = 0;
> +		return NULL;
> +	}
>  	if (sizep)
>  		*sizep = v->size;
>  	return v->buffer;

I think you need matching changes in unused_commit_buffer and free_commit_buffer. And detach_commit_buffer, too, I guess. Basically everywhere except set_commit_buffer would want to use the peek version.

-Peff