Re: [PATCH v2 5/5] Expand implementation of mem-pool type

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 23, 2018, 20:41 UTC
Message-ID: <xmqqk1u2ikxt.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180323144408.213145-6-jamill@microsoft.com>

Jameson Miller <jamill@microsoft.com> writes:

Show 12 quoted lines

> +void mem_pool_discard(struct mem_pool *mem_pool)
> +{
> +	struct mp_block *block, *block_to_free;
> +	for (block = mem_pool->mp_block; block;)
> +	{
> +		block_to_free = block;
> +		block = block->next_block;
> +		free(block_to_free);
> +	}
> +
> +	free(mem_pool);
> +}

This can be used as a (half-)valid justification to the unadvertised behaviour change made in [2/5] where a large allocation is still given its own block and connected to a pool (as opposed to bypassing the pool subsystem altogether). If an instance of pool does not keep track of all the pieces of memory mem_pool_alloc() hands out, mem_pool_discard() cannot be used to reclaim all of the resources.

It however still does not justify how pool_alloc_block_with_size() appends the new block at the end of the "next" list, which is scanned for unused spaces when a new request comes in. "We need to keep track of them so that disard() can release" would justify a pointer in the pool struct that chains together allocation blocks, each of which hosts the memory for a single large allocation request without extra room to carve out memory for subsequent small requests. That pointer does not have to be the usual "mp_block" pointer.

Show 40 quoted lines

> diff --git a/mem-pool.h b/mem-pool.h
> index 829ad58ecf..d9e7f21541 100644
> --- a/mem-pool.h
> +++ b/mem-pool.h
> @@ -21,6 +21,17 @@ struct mem_pool {
>  	size_t pool_alloc;
>  };
>  
> +/*
> + * Initialize mem_pool with specified parameters for initial size and
> + * how much to grow when a larger memory block is required.
> + */
> +void mem_pool_init(struct mem_pool **mem_pool, size_t alloc_growth_size, size_t initial_size);
> +
> +/*
> + * Discard a memory pool and free all the memory it is responsible for.
> + */
> +void mem_pool_discard(struct mem_pool *mem_pool);
> +
>  /*
>   * Alloc memory from the mem_pool.
>   */
> @@ -31,4 +42,17 @@ void *mem_pool_alloc(struct mem_pool *pool, size_t len);
>   */
>  void *mem_pool_calloc(struct mem_pool *pool, size_t count, size_t size);
>  
> +/*
> + * Move the memory associated with the 'src' pool to the 'dst' pool. The 'src'
> + * pool will be empty and not contain any memory. It still needs to be free'd
> + * with a call to `mem_pool_discard`.
> + */
> +void mem_pool_combine(struct mem_pool *dst, struct mem_pool *src);
> +
> +/*
> + * Check if a memory pointed at by 'mem' is part of the range of
> + * memory managed by the specified mem_pool.
> + */
> +int mem_pool_contains(struct mem_pool *mem_pool, void *mem);
> +
>  #endif

When adding a new "API", we prefer to see its usage demonstrated with its first user, either in the same patch or in later steps in the same series. That would make it easier to evaluate its worth fairly.

Thanks.