From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 423374765E0 for ; Thu, 24 Dec 2020 18:13:29 +0300 (MSK) References: <20201223131452.23424-1-mechanik20051988@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Thu, 24 Dec 2020 16:13:27 +0100 MIME-Version: 1.0 In-Reply-To: <20201223131452.23424-1-mechanik20051988@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] memtx: change small allocator behavior List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mechanik20051988 Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the fixes! On 23.12.2020 14:14, mechanik20051988 via Tarantool-patches wrote: > From: mechanik20051988 > > 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.