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.
next prev parent 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