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

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

Hi! Thanks for the patch!

Something seems to be broken in your editor or whatever
you use to prepare emails.

This thread has 2 commits, while really your have 4 commits -
1 in tarantool/tarantool and 3 in tarantool/small.

Consider my review fixes on top of this branch in a separate
commit. Squash them if all is fine.

On 25.12.2020 14:45, 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);

All the messages run out of 80 symbols line width limit, which is
stated here: https://github.com/tarantool/tarantool/wiki/Code-review-procedure.
I see you still didn't read it, right? I suggest to do it, otherwise you
are going to face all the problems listed in there, which will make the
reviews longer, and will waste more time.

I also renamed 'alloc_factor' to 'slab_alloc_factor' to be consistent
with box.cfg.slab_alloc_factor.

My fixes, also in a separate commit on top of your branch.

====================
diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 023402888..f79f14b4f 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -1117,10 +1117,12 @@ memtx_engine_new(const char *snap_dirname, bool force_recovery,
 		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");
+		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");
+		say_error("Alloc factor must be greater than 1.0. It will be "
+			  "increased to 1.001");
 		alloc_factor = 1.001;
 	}
 
@@ -1132,7 +1134,8 @@ memtx_engine_new(const char *snap_dirname, bool force_recovery,
 	float actual_alloc_factor;
 	small_alloc_create(&memtx->alloc, &memtx->slab_cache,
 			   objsize_min, alloc_factor, &actual_alloc_factor);
-	say_info("Actual alloc factor calculated on the basis of desired alloc_factor = %f", actual_alloc_factor);
+	say_info("Actual slab_alloc_factor calculated on the basis of desired "
+		 "slab_alloc_factor = %f", actual_alloc_factor);
 
 	/* Initialize index extent allocator. */
 	slab_cache_create(&memtx->index_slab_cache, &memtx->arena);