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

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

Hi! Thanks for the fixes!

On 23.12.2020 14:14, 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
> Pull request: https://github.com/tarantool/tarantool/pull/5503
> 
> Thank you for the previous review. Here some answers on you questions.

I don't remember the questions by their numbers. I enumerate them so as
people wouldn't miss any of the comments. For example, when I say 'see 10
comments below', it is supposed to be easier to check if you didn't miss,
say, comment 5, between comments 4 and 6.

Try to follow this guide:
https://github.com/tarantool/tarantool/wiki/Code-review-procedure#during-the-review

Responses better be right under the questions, in response to the original
email in the old thread, with diff under each comment.

> 2. As you can see at this moment, small_alloc_create already 
>    has restrictions on alloc_factor (see small.c in master branch)
>    They change it silently. 
>    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;
> 	}
>   I only moved them in memtx_engine.c with an explicit message.

Ok, I see now.

See 3 comments below.

>  src/box/memtx_engine.c                  |  8 +++
>  src/lib/small                           |  2 +-
>  test/engine/engine.cfg                  |  3 ++
>  test/engine/small_alloc_factor.lua      | 11 ++++
>  test/engine/small_alloc_factor.result   | 70 +++++++++++++++++++++++++
>  test/engine/small_alloc_factor.test.lua | 20 +++++++
>  6 files changed, 113 insertions(+), 1 deletion(-)
>  create mode 100644 test/engine/small_alloc_factor.lua
>  create mode 100644 test/engine/small_alloc_factor.result
>  create mode 100644 test/engine/small_alloc_factor.test.lua
> 
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index 4f1fe3a3f..3ab292f2e 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -1116,6 +1116,14 @@ 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 then 1.0. It will be increased to 1.001");

1. Can't parse 'than then' - please, rephrase. I assume 'then' is
not needed.

> +		alloc_factor = 1.001;

2. I don't see the factor being rounded up in the original code.
Why did you add it? And why 1.0 is bad?

> +	}
> diff --git a/test/engine/small_alloc_factor.result b/test/engine/small_alloc_factor.result
> new file mode 100644
> index 000000000..31d0f8ffd
> --- /dev/null
> +++ b/test/engine/small_alloc_factor.result

3. Please, read this document, and especially the specified
section: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#testing.
The test file names should have a special pattern. Please, follow it.