From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 A236B4765E0 for ; Mon, 28 Dec 2020 15:11:39 +0300 (MSK) References: <20201225114520.18796-1-mechanik20051988@tarantool.org> From: Vladislav Shpilevoy Message-ID: <9473a71c-2c1b-dd9f-8988-8cee883a31f6@tarantool.org> Date: Mon, 28 Dec 2020 15:11:37 +0300 MIME-Version: 1.0 In-Reply-To: <20201225114520.18796-1-mechanik20051988@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v4] memtx: change small allocator behavior List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mechanik20051988 Cc: mechanik20051988 , tarantool-patches@dev.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 > > 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);