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

[Junio C Hamano <gitster@pobox.com>]

Jameson Miller <jamill@microsoft.com> writes:

> 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.

> @@ -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.

> @@ -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.

> +
> +	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.

> -		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;
>  }