Re: [tarantool-patches] [PATCH v3 1/7] memtx: introduce universal iterator_pool

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Sat, Feb 23, 2019 at 03:03:02PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/memtx_bitset.c b/src/box/memtx_bitset.c
> index cd7362ee1..9dbc4141d 100644
> --- a/src/box/memtx_bitset.c
> +++ b/src/box/memtx_bitset.c
> @@ -162,6 +162,10 @@ struct bitset_index_iterator {
>  	struct mempool *pool;
>  };
>  
> +static_assert(sizeof(struct bitset_index_iterator) <= MEMTX_ITERATOR_SIZE,
> +	      "bitset_index_iterator must be less or equal than "

"sizeof(struct bitset_index_iterator) must be less than or equal to MEMTX_ITERATOR_SIZE"

Ditto for other static assertions.

> +	      "MEMTX_ITERATOR_SIZE");
> +
>  static struct bitset_index_iterator *
>  bitset_index_iterator(struct iterator *it)
>  {
> @@ -493,9 +497,9 @@ memtx_bitset_index_new(struct memtx_engine *memtx, struct index_def *def)
>  	assert(def->iid > 0);
>  	assert(!def->opts.is_unique);
>  
> -	if (!mempool_is_initialized(&memtx->bitset_iterator_pool)) {
> -		mempool_create(&memtx->bitset_iterator_pool, cord_slab_cache(),
> -			       sizeof(struct bitset_index_iterator));
> +	if (!mempool_is_initialized(&memtx->iterator_pool)) {
> +		mempool_create(&memtx->iterator_pool, cord_slab_cache(),
> +			       MEMTX_ITERATOR_SIZE);

Since there's one pool for bitset, hash, and tree indexes, size of which
is known in advance, we can initialize it in the engine constructor and
destroy it unconditionally in the engine destructor.

> diff --git a/src/box/memtx_engine.h b/src/box/memtx_engine.h
> index 0f8e92ee4..5859390d8 100644
> --- a/src/box/memtx_engine.h
> +++ b/src/box/memtx_engine.h
> @@ -87,6 +87,14 @@ enum memtx_recovery_state {
>  /** Memtx extents pool, available to statistics. */
>  extern struct mempool memtx_index_extent_pool;
>  
> +/**
> + * The size of the biggest memtx iterator. Used with
> + * mempool_create. This is the size of the block that will be
> + * allocated for each iterator (except rtree index iterator that
> + * is significantly bigger so has own pool).
> + */
> +#define MEMTX_ITERATOR_SIZE (696)

Should be about 150 bytes without rtree, no?

> +
>  struct memtx_engine {
>  	struct engine base;
>  	/** Engine recovery state. */
> @@ -129,14 +137,10 @@ struct memtx_engine {
>  	size_t max_tuple_size;
>  	/** Incremented with each next snapshot. */
>  	uint32_t snapshot_version;
> -	/** Memory pool for tree index iterator. */
> -	struct mempool tree_iterator_pool;
>  	/** Memory pool for rtree index iterator. */
>  	struct mempool rtree_iterator_pool;
> -	/** Memory pool for hash index iterator. */
> -	struct mempool hash_iterator_pool;
> -	/** Memory pool for bitset index iterator. */
> -	struct mempool bitset_iterator_pool;
> +	/** Memory pool for index iterator. */

Memory pool for all index iterators except rtree.
The latter is significantly larger so it has its
own memory pool.

> +	struct mempool iterator_pool;