From: "Evgeny Mekhanik" <mechanik20051988@tarantool.org> To: "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] FW: [PATCH] memtx: change small allocator behavior Date: Fri, 25 Dec 2020 10:42:44 +0300 [thread overview] Message-ID: <1608882164.215919067@f40.i.mail.ru> (raw) In-Reply-To: <0076A088-8CBC-4238-9EEB-0C73EC516098@hxcore.ol> [-- Attachment #1: Type: text/plain, Size: 6520 bytes --] Hi, thank you for review.I answer your questions below. 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. Fixed incorrect message, now: + 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; + } + > + 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? Answer: Here is a code which rounded up alloc factor in master branch. /* * 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 use gdb to show it: tarantool> box.cfg{slab_alloc_factor = 1.01} 2020-12-24 19:22:33.416 [22602] main/103/interactive C> Tarantool 2.7.0-148-gd5a5754a3 2020-12-24 19:22:33.416 [22602] main/103/interactive C> log level 5 2020-12-24 19:22:33.416 [22602] main/103/interactive I> mapping 268435456 bytes for memtx tuple arena... Breakpoint 1, small_alloc_create (alloc=0x555555cfe788, cache=0x555555cfe530, objsize_min=272, alloc_factor=1.00999999) at /home/evgeny/MASTER/tarantool/src/lib/small/small/small.c:135 135 alloc_factor = (gdb) n 139 alloc->factor = alloc_factor; (gdb) p alloc_factor $1 = 1.032197 (gdb) As you can see if you pass slab_alloc_factor < 1.03 it's changed to 1.03. I try 0.01 factor and get the same value. It's code from master branch, now user can pass >= 1.001 factor. We can't use 1.0, because we need increment to pool size. > + } > 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. Rename test, add necessary gh-5216 (issue number) prefix diff --git a/test/engine/gh-5216-small_alloc_factor.lua b/test/engine/gh-5216-small_alloc_factor.lua +#!/usr/bin/env tarantool + +require('console').listen(os.getenv('ADMIN')) + +box.cfg({ + wal_mode = 'none', + memtx_memory = 2*1024*1024*1024, + listen = os.getenv("LISTEN"), + memtx_min_tuple_size=32, + slab_alloc_factor=1.03 +}) diff --git a/test/engine/gh-5216-small_alloc_factor.result b/test/engine/gh-5216-small_alloc_factor.result +env = require('test_run') +--- +... +test_run = env.new() +--- +... +test_run:cmd('create server test with script="engine/gh-5216-small_alloc_factor.lua"') +--- +- true +... +test_run:cmd('start server test with args="none"') +--- +- true +... +test_run:cmd('switch test') +--- +- true +... +s = box.schema.space.create('test') +--- +... +_ = s:create_index('test') +--- +... +function str(i) return string.rep('a', math.floor(256 * math.pow(1.03, i))) end +--- +... +for i=1,276 do _ = s:replace{i+200, str(i)} end +--- +... +for i=1,274 do _ = s:delete{i+200} end +--- +... +collectgarbage('collect') +--- +- 0 +... +_ = s:replace{200+277, str(275)} +--- +... +_ = s:delete{200+275} +--- +... +collectgarbage('collect') +--- +- 0 +... +_ = s:delete{200+276} +--- +... +collectgarbage('collect') +--- +- 0 +... +test_run:cmd('switch default') +--- +- true +... +test_run:cmd('stop server test') +--- +- true +... +test_run:cmd('cleanup server test') +--- +- true +... +test_run:cmd('delete server test') +--- +- true +... diff --git a/test/engine/gh-5216-small_alloc_factor.test.lua b/test/engine/gh-5216-small_alloc_factor.test.lua +env = require('test_run') +test_run = env.new() +test_run:cmd('create server test with script="engine/gh-5216-small_alloc_factor.lua"') +test_run:cmd('start server test with args="none"') +test_run:cmd('switch test') +s = box.schema.space.create('test') +_ = s:create_index('test') +function str(i) return string.rep('a', math.floor(256 * math.pow(1.03, i))) end +for i=1,276 do _ = s:replace{i+200, str(i)} end +for i=1,274 do _ = s:delete{i+200} end +collectgarbage('collect') +_ = s:replace{200+277, str(275)} +_ = s:delete{200+275} +collectgarbage('collect') +_ = s:delete{200+276} +collectgarbage('collect') +test_run:cmd('switch default') +test_run:cmd('stop server test') +test_run:cmd('cleanup server test') +test_run:cmd('delete server test') [-- Attachment #2: Type: text/html, Size: 10932 bytes --]
next prev parent reply other threads:[~2020-12-25 7:42 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 [not found] ` <0076A088-8CBC-4238-9EEB-0C73EC516098@hxcore.ol> 2020-12-25 7:42 ` Evgeny Mekhanik [this message] 2020-12-28 12:10 ` [Tarantool-patches] FW: " 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=1608882164.215919067@f40.i.mail.ru \ --to=mechanik20051988@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] FW: [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