Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: mechanik20051988 <mechanik20.05.1988@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] memtx: change small allocator behavior
Date: Thu, 24 Dec 2020 16:13:27 +0100	[thread overview]
Message-ID: <fa552e40-5ddd-ac82-5dfc-0bb6600ea8bf@tarantool.org> (raw)
In-Reply-To: <20201223131452.23424-1-mechanik20051988@gmail.com>

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.

  reply	other threads:[~2020-12-24 15:13 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-23 13:14 [Tarantool-patches] [PATCH v2 0/3] " mechanik20051988
2020-12-23 13:14 ` [Tarantool-patches] [PATCH] memtx: " mechanik20051988
2020-12-24 15:13   ` Vladislav Shpilevoy [this message]
     [not found]     ` <0076A088-8CBC-4238-9EEB-0C73EC516098@hxcore.ol>
2020-12-25  7:42       ` [Tarantool-patches] FW: " Evgeny Mekhanik
2020-12-28 12:10         ` Vladislav Shpilevoy
2020-12-23 13:14 ` [Tarantool-patches] [PATCH v2 1/3] small: implement new size class evaluation mechanik20051988
2020-12-24 15:13   ` Vladislav Shpilevoy
     [not found]     ` <A90D94B7-298A-4D1B-8134-6EE2ED45D615@hxcore.ol>
2020-12-25  7:48       ` [Tarantool-patches] FW: " Evgeny Mekhanik
2020-12-28 12:10     ` [Tarantool-patches] " Vladislav Shpilevoy
2020-12-23 13:14 ` [Tarantool-patches] [PATCH v2 2/3] test: add small allocator performance test mechanik20051988
2020-12-23 13:14 ` [Tarantool-patches] [PATCH v2 3/3] small: changed small allocator pool management mechanik20051988
2020-12-24 15:13   ` Vladislav Shpilevoy
     [not found]     ` <27E47303-4307-4713-BB8A-2427FED09DDE@hxcore.ol>
2020-12-25  7:52       ` [Tarantool-patches] FW: " Evgeny Mekhanik
2020-12-25  7:56       ` Evgeny Mekhanik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fa552e40-5ddd-ac82-5dfc-0bb6600ea8bf@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=mechanik20.05.1988@gmail.com \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] memtx: change small allocator behavior' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox