Re: [Tarantool-patches] [PATCH 3/3] Changed small allocator pool management

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 4 comments below.

On 18.12.2020 14:35, mechanik20051988 wrote:
> From: mechanik20051988 <mechanik20.05.1988@gmail.com>
> 
> In previous version allocator create new pool if necessary
> and insert it in the pools tree. Now we allocate pools on

1. create -> created, insert -> inserted.

> the stage of allocator creation according alloc_factor.
> We use small_alloc class for this purpose also we use it
> to find necessary pool when we alloc memory. This is faster
> then previous allocator behavior and also fixes #5216.
> 
> Closes #5216
> ---
>  small/small.c | 212 ++++++++++++--------------------------------------
>  small/small.h |  47 ++++-------
>  2 files changed, 62 insertions(+), 197 deletions(-)
> 
> diff --git a/small/small.c b/small/small.c
> index 48085fb..2196906 100644
> --- a/small/small.c
> +++ b/small/small.c
> @@ -102,53 +70,17 @@ small_alloc_create(struct small_alloc *alloc, struct slab_cache *cache,
>  {
>  	alloc->cache = cache;
>  	/* Align sizes. */
> -	objsize_min = small_align(objsize_min, STEP_SIZE);
> -	alloc->step_pool0_step_count = (objsize_min - 1) >> STEP_SIZE_LB;
> +	objsize_min = small_align(objsize_min, sizeof(intptr_t));
>  	/* Make sure at least 4 largest objects can fit in a slab. */
>  	alloc->objsize_max =
>  		mempool_objsize_max(slab_order_size(cache, cache->order_max));
>  
> -	if (!(alloc->objsize_max > objsize_min + STEP_POOL_MAX * STEP_SIZE)) {
> -		fprintf(stderr, "Can't create small alloc, small "
> -			"object min size should not be greather than %u\n",
> -			alloc->objsize_max - (STEP_POOL_MAX + 1) * STEP_SIZE);
> -		abort();
> -	}
> +	assert(alloc_factor > 1. && alloc_factor <= 2.);
>  
> -	struct mempool *step_pool;
> -	for (step_pool = alloc->step_pools;
> -	     step_pool < alloc->step_pools + STEP_POOL_MAX;
> -	     step_pool++) {
> -		mempool_create(step_pool, alloc->cache, objsize_min);
> -		objsize_min += STEP_SIZE;
> -	}
> -	alloc->step_pool_objsize_max = (step_pool - 1)->objsize;
> -	if (alloc_factor > 2.0)
> -		alloc_factor = 2.0;
> -	/*
> -	 * Correct the user-supplied alloc_factor to ensure that
> -	 * it actually produces growing object sizes.
> -	 */
> -	if (alloc->step_pool_objsize_max * alloc_factor <
> -	    alloc->step_pool_objsize_max + STEP_SIZE) {
> -
> -		alloc_factor =
> -			(alloc->step_pool_objsize_max + STEP_SIZE + 0.5)/
> -			alloc->step_pool_objsize_max;
> -	}
>  	alloc->factor = alloc_factor;
> -
> -	/* Initialize the factored pool cache. */
> -	struct factor_pool *factor_pool = alloc->factor_pool_cache;
> -	do {
> -		factor_pool->next = factor_pool + 1;
> -		factor_pool++;
> -	} while (factor_pool !=
> -		 alloc->factor_pool_cache + FACTOR_POOL_MAX - 1);
> -	factor_pool->next = NULL;
> -	alloc->factor_pool_next = alloc->factor_pool_cache;
> -	factor_tree_new(&alloc->factor_pools);
> -	(void) factor_pool_create(alloc, NULL, alloc->objsize_max);
> +	small_class_create(&alloc->small_class, sizeof(intptr_t),
> +		alloc->factor, objsize_min);

2. Broken alignment.

> +	factor_pool_create(alloc);
>  
>  	lifo_init(&alloc->delayed);
>  	lifo_init(&alloc->delayed_large);
> @@ -360,16 +244,16 @@ mempool_iterator_create(struct mempool_iterator *it,
>  			struct small_alloc *alloc)
>  {
>  	it->alloc = alloc;
> -	it->step_pool = alloc->step_pools;
> -	factor_tree_ifirst(&alloc->factor_pools, &it->factor_iterator);
> +	it->factor_iterator = 0;
>  }
>  
>  struct mempool *
>  mempool_iterator_next(struct mempool_iterator *it)
>  {
> -	if (it->step_pool < it->alloc->step_pools + STEP_POOL_MAX)
> -		return it->step_pool++;
> -	struct factor_pool *factor_pool = factor_tree_inext(&it->factor_iterator);
> +	struct factor_pool *factor_pool = NULL;
> +	if (it->factor_iterator < it->alloc->factor_pool_cache_size)
> +		factor_pool =
> +			&it->alloc->factor_pool_cache[(it->factor_iterator)++];

3. When 'if' body has more than 1 line, it must be in {}.

>  	if (factor_pool) {
>  		return &(factor_pool->pool);
>  	}
> @@ -387,7 +271,7 @@ small_alloc_destroy(struct small_alloc *alloc)
>  		mempool_destroy(pool);
>  	}
>  	lifo_init(&alloc->delayed);
> -
> +	alloc->factor_pool_cache_size = 0;

4. Why is it needed at destruction?

>  	/* Free large allocations */
>  	void *item;
>  	while ((item = lifo_pop(&alloc->delayed_large))) {