Re: [Tarantool-patches] [PATCH v4] memtx: change small allocator behavior

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

Hi Evgeny, thanks for the patchset, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patchset LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/235049916

On Fri, Dec 25, 2020 at 02:45:19PM +0300, mechanik20051988 via Tarantool-patches wrote:
> From: mechanik20051988 <mechanik20.05.1988@gmail.com>
> 
> Branch: https://github.com/tarantool/tarantool/tree/mechanik20051988/gh-5216-fix-strange-allocator-behavior
> 
> Cnanges in v4:
> 	Add actual_alloc_factor parameter to small_alloc_create
> 
> Previously, in small allocator, memory pools
> were allocated at the request, which in the case
> of a small slab_alloc_factor led to use
> pools with incorrect sizes. This patch changed
> small allocator behavior, now we allocate pools
> on the stage of allocator creation. Also we use
> special function to find appropriate pool, which
> is faster, then previous version with rbtree.
> This change fixes #5216.
> 
> Also moved a check, that the slab_alloc_factor is in
> the range (1.0, 2.0] from small allocator to memtx_engine.
> If factor is not in range change it to 1.0001 or 2.0 respectively
> 
> Closes #5216
> ---
>  src/box/memtx_engine.c                        | 12 +++-
>  src/box/tuple.c                               |  3 +-
>  src/lib/small                                 |  2 +-
>  test/engine/engine.cfg                        |  3 +
>  test/engine/gh-5216-small_alloc_factor.lua    | 11 +++
>  test/engine/gh-5216-small_alloc_factor.result | 70 +++++++++++++++++++
>  .../gh-5216-small_alloc_factor.test.lua       | 20 ++++++
>  7 files changed, 118 insertions(+), 3 deletions(-)
>  create mode 100644 test/engine/gh-5216-small_alloc_factor.lua
>  create mode 100644 test/engine/gh-5216-small_alloc_factor.result
>  create mode 100644 test/engine/gh-5216-small_alloc_factor.test.lua
> 
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index 4f1fe3a3f..023402888 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -1116,13 +1116,23 @@ memtx_engine_new(const char *snap_dirname, bool force_recovery,
>  	if (objsize_min < OBJSIZE_MIN)
>  		objsize_min = OBJSIZE_MIN;
>  
> +	if (alloc_factor > 2) {
> +		say_error("Alloc factor must be less than or equal to 2.0. It will be reduced to 2.0");
> +		alloc_factor = 2.0;
> +	} else if (alloc_factor <= 1.0) {
> +		say_error("Alloc factor must be greater than 1.0. It will be increased to 1.001");
> +		alloc_factor = 1.001;
> +	}
> +
>  	/* Initialize tuple allocator. */
>  	quota_init(&memtx->quota, tuple_arena_max_size);
>  	tuple_arena_create(&memtx->arena, &memtx->quota, tuple_arena_max_size,
>  			   SLAB_SIZE, dontdump, "memtx");
>  	slab_cache_create(&memtx->slab_cache, &memtx->arena);
> +	float actual_alloc_factor;
>  	small_alloc_create(&memtx->alloc, &memtx->slab_cache,
> -			   objsize_min, alloc_factor);
> +			   objsize_min, alloc_factor, &actual_alloc_factor);
> +	say_info("Actual alloc factor calculated on the basis of desired alloc_factor = %f", actual_alloc_factor);
>  
>  	/* Initialize index extent allocator. */
>  	slab_cache_create(&memtx->index_slab_cache, &memtx->arena);
> diff --git a/src/box/tuple.c b/src/box/tuple.c
> index e83377c3a..c2023e4e8 100644
> --- a/src/box/tuple.c
> +++ b/src/box/tuple.c
> @@ -313,8 +313,9 @@ tuple_init(field_name_hash_f hash)
>  	/* Make sure this one stays around. */
>  	tuple_format_ref(tuple_format_runtime);
>  
> +	float actual_alloc_factor;
>  	small_alloc_create(&runtime_alloc, &cord()->slabc, OBJSIZE_MIN,
> -			   ALLOC_FACTOR);
> +			   ALLOC_FACTOR, &actual_alloc_factor);
>  
>  	mempool_create(&tuple_iterator_pool, &cord()->slabc,
>  		       sizeof(struct tuple_iterator));
> diff --git a/src/lib/small b/src/lib/small
> index fcac155db..bdbe73cd3 160000
> --- a/src/lib/small
> +++ b/src/lib/small
> @@ -1 +1 @@
> -Subproject commit fcac155dba18e97b10bc668d1a2fae01184adc13
> +Subproject commit bdbe73cd35865160dd9f80bfc8bfa3817f4980aa
> diff --git a/test/engine/engine.cfg b/test/engine/engine.cfg
> index f017a03ec..3bfc44dcf 100644
> --- a/test/engine/engine.cfg
> +++ b/test/engine/engine.cfg
> @@ -8,6 +8,9 @@
>      },
>      "gh-4973-concurrent-alter-fails.test.lua": {
>          "memtx": {"engine": "memtx"}
> +    },
> +    "gh-5216-small_alloc_factor.test.lua" : {
> +        "memtx": {"engine": "memtx"}
>      }
>  }
>  
> diff --git a/test/engine/gh-5216-small_alloc_factor.lua b/test/engine/gh-5216-small_alloc_factor.lua
> new file mode 100644
> index 000000000..a303a95d4
> --- /dev/null
> +++ b/test/engine/gh-5216-small_alloc_factor.lua
> @@ -0,0 +1,11 @@
> +#!/usr/bin/env tarantool
> +
> +require('console').listen(os.getenv('ADMIN'))
> +
> +box.cfg({
> +    wal_mode = 'none',
> +    memtx_memory = 2*1024*1024*1024,
> +    listen = os.getenv("LISTEN"),
> +    memtx_min_tuple_size=32,
> +    slab_alloc_factor=1.03
> +})
> diff --git a/test/engine/gh-5216-small_alloc_factor.result b/test/engine/gh-5216-small_alloc_factor.result
> new file mode 100644
> index 000000000..5cf69a8f3
> --- /dev/null
> +++ b/test/engine/gh-5216-small_alloc_factor.result
> @@ -0,0 +1,70 @@
> +env = require('test_run')
> +---
> +...
> +test_run = env.new()
> +---
> +...
> +test_run:cmd('create server test with script="engine/gh-5216-small_alloc_factor.lua"')
> +---
> +- true
> +...
> +test_run:cmd('start server test with args="none"')
> +---
> +- true
> +...
> +test_run:cmd('switch test')
> +---
> +- true
> +...
> +s = box.schema.space.create('test')
> +---
> +...
> +_ = s:create_index('test')
> +---
> +...
> +function str(i) return string.rep('a', math.floor(256 * math.pow(1.03, i))) end
> +---
> +...
> +for i=1,276 do _ = s:replace{i+200, str(i)} end
> +---
> +...
> +for i=1,274 do _ = s:delete{i+200} end
> +---
> +...
> +collectgarbage('collect')
> +---
> +- 0
> +...
> +_ = s:replace{200+277, str(275)}
> +---
> +...
> +_ = s:delete{200+275}
> +---
> +...
> +collectgarbage('collect')
> +---
> +- 0
> +...
> +_ = s:delete{200+276}
> +---
> +...
> +collectgarbage('collect')
> +---
> +- 0
> +...
> +test_run:cmd('switch default')
> +---
> +- true
> +...
> +test_run:cmd('stop server test')
> +---
> +- true
> +...
> +test_run:cmd('cleanup server test')
> +---
> +- true
> +...
> +test_run:cmd('delete server test')
> +---
> +- true
> +...
> diff --git a/test/engine/gh-5216-small_alloc_factor.test.lua b/test/engine/gh-5216-small_alloc_factor.test.lua
> new file mode 100644
> index 000000000..83e51d19c
> --- /dev/null
> +++ b/test/engine/gh-5216-small_alloc_factor.test.lua
> @@ -0,0 +1,20 @@
> +env = require('test_run')
> +test_run = env.new()
> +test_run:cmd('create server test with script="engine/gh-5216-small_alloc_factor.lua"')
> +test_run:cmd('start server test with args="none"')
> +test_run:cmd('switch test')
> +s = box.schema.space.create('test')
> +_ = s:create_index('test')
> +function str(i) return string.rep('a', math.floor(256 * math.pow(1.03, i))) end
> +for i=1,276 do _ = s:replace{i+200, str(i)} end
> +for i=1,274 do _ = s:delete{i+200} end
> +collectgarbage('collect')
> +_ = s:replace{200+277, str(275)}
> +_ = s:delete{200+275}
> +collectgarbage('collect')
> +_ = s:delete{200+276}
> +collectgarbage('collect')
> +test_run:cmd('switch default')
> +test_run:cmd('stop server test')
> +test_run:cmd('cleanup server test')
> +test_run:cmd('delete server test')
> -- 
> 2.20.1
>