Re: [PATCH v3 2/3] fast-import: introduce mem_pool type

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 27, 2018, 16:09 UTC
Message-ID: <xmqqpo3pa4ag.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180326170351.197793-3-jamill@microsoft.com>

Jameson Miller <jamill@microsoft.com> writes:

Show 13 quoted lines

> Introduce the mem_pool type which encapsulates all the information
> necessary to manage a pool of memory.This change moves the existing
> variables in fast-import used to support the global memory pool to use
> this structure.
>
> These changes allow for the multiple instances of a memory pool to
> exist and be reused outside of fast-import. In a future commit the
> mem_pool type will be moved to its own file.
>
> Signed-off-by: Jameson Miller <jamill@microsoft.com>
> ---
>  fast-import.c | 80 +++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 51 insertions(+), 29 deletions(-)

Thanks, this got much easier to read and reason about.

Show 16 quoted lines

> @@ -304,9 +317,7 @@ static int global_argc;
>  static const char **global_argv;
>  
>  /* Memory pools */
> -static size_t mem_pool_alloc = 2*1024*1024 - sizeof(struct mp_block);
> -static size_t total_allocd;
> -static struct mp_block *mp_block_head;
> +static struct mem_pool fi_mem_pool =  {0, 2*1024*1024 - sizeof(struct mp_block), 0 };
>  
>  /* Atom management */
>  static unsigned int atom_table_sz = 4451;
> @@ -324,6 +335,7 @@ static off_t pack_size;
>  /* Table of objects we've written. */
>  static unsigned int object_entry_alloc = 5000;
>  static struct object_entry_pool *blocks;
> +static size_t total_allocd;

So the design decision made at this step is that pool_alloc field keeps track of the per-pool allocation, while total_allocd is a sum across instances of pools. That sounds appropriate for stats.

Show 15 quoted lines

> @@ -634,7 +646,21 @@ static unsigned int hc_str(const char *s, size_t len)
>  	return r;
>  }
>  
> -static void *pool_alloc(size_t len)
> +static struct mp_block *mem_pool_alloc_block(struct mem_pool *mem_pool, size_t block_alloc)
> +{
> +	struct mp_block *p;
> +
> +	mem_pool->pool_alloc += sizeof(struct mp_block) + block_alloc;
> +	p = xmalloc(st_add(sizeof(struct mp_block), block_alloc));
> +	p->next_block = mem_pool->mp_block;
> +	p->next_free = (char *)p->space;
> +	p->end = p->next_free + block_alloc;
> +	mem_pool->mp_block = p;

This, compared to what used to happen in mem_pool_alloc()'s original code, ignores total_allocd. I cannot tell if that is an intentional change or a mistake.

Show 25 quoted lines

> +
> +	return p;
> +}
> +
> +static void *mem_pool_alloc(struct mem_pool *mem_pool, size_t len)
>  {
>  	struct mp_block *p;
>  	void *r;
> @@ -643,21 +669,17 @@ static void *pool_alloc(size_t len)
>  	if (len & (sizeof(uintmax_t) - 1))
>  		len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));
>  
> -	for (p = mp_block_head; p; p = p->next_block)
> -		if ((p->end - p->next_free >= len))
> -			break;
> +	for (p = mem_pool->mp_block; p; p = p->next_block)
> +		if (p->end - p->next_free >= len)
> +	       		break;
>  
>  	if (!p) {
> -		if (len >= (mem_pool_alloc/2)) {
> -			total_allocd += len;
> +		if (len >= (mem_pool->block_alloc / 2)) {
> +			mem_pool->pool_alloc += len;
>  			return xmalloc(len);

It is unfair to account this piece of memory to the mem_pool, as we ended up not carving it out from here. Did you mean to increment total_allocd by len instead, perhaps?

And I do agree with the idea in the previous round to make these oversized pieces of memory allocated here to be freeable via the mem_pool instance (I only found it questionable to use the main "here are the list of blocks that we could carve small pieces out" list), and anticipate that a later step in the series would change this part to do so. With that anticipation, I very much appreciate that this step did not mix that and stayed as close to the original (except for the possible mis-accounting). It makes it very clear what is going on in each separate step in the series.

>  		}
> -		total_allocd += sizeof(struct mp_block) + mem_pool_alloc;

This is what I noticed got lost in the pool-alloc-block helper above.

Show 20 quoted lines

> -		p = xmalloc(st_add(sizeof(struct mp_block), mem_pool_alloc));
> -		p->next_block = mp_block_head;
> -		p->next_free = (char *) p->space;
> -		p->end = p->next_free + mem_pool_alloc;
> -		mp_block_head = p;
> +
> +		p = mem_pool_alloc_block(mem_pool, mem_pool->block_alloc);
>  	}
>  
>  	r = p->next_free;
> @@ -665,10 +687,10 @@ static void *pool_alloc(size_t len)
>  	return r;
>  }
>  
> -static void *pool_calloc(size_t count, size_t size)
> +static void *mem_pool_calloc(struct mem_pool *mem_pool, size_t count, size_t size)
>  {
> -	size_t len = count * size;
> -	void *r = pool_alloc(len);
> +	size_t len = st_mult(count, size);

Nice ;-)

> +	void *r = mem_pool_alloc(mem_pool, len);
>  	memset(r, 0, len);
>  	return r;
>  }