<HTML><BODY><div>Hi, thank you for review.I answer your questions below.</div><div> </div><div>See 3 comments below.</div><div><div id=""><div class="js-helper js-readmsg-msg"><div id="style_16088813761179004474_BODY"><div class="cl_911316"><div class="WordSection1_mr_css_attr"><p class="MsoNormal_mr_css_attr"> </p><p class="MsoNormal_mr_css_attr">> src/box/memtx_engine.c | 8 +++</p><p class="MsoNormal_mr_css_attr">> src/lib/small | 2 +-</p><p class="MsoNormal_mr_css_attr">> test/engine/engine.cfg | 3 ++</p><p class="MsoNormal_mr_css_attr">> test/engine/small_alloc_factor.lua | 11 ++++</p><p class="MsoNormal_mr_css_attr">> test/engine/small_alloc_factor.result | 70 +++++++++++++++++++++++++</p><p class="MsoNormal_mr_css_attr">> test/engine/small_alloc_factor.test.lua | 20 +++++++</p><p class="MsoNormal_mr_css_attr">> 6 files changed, 113 insertions(+), 1 deletion(-)</p><p class="MsoNormal_mr_css_attr">> create mode 100644 test/engine/small_alloc_factor.lua</p><p class="MsoNormal_mr_css_attr">> create mode 100644 test/engine/small_alloc_factor.result</p><p class="MsoNormal_mr_css_attr">> create mode 100644 test/engine/small_alloc_factor.test.lua</p><p class="MsoNormal_mr_css_attr">></p><p class="MsoNormal_mr_css_attr">> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c</p><p class="MsoNormal_mr_css_attr">> index 4f1fe3a3f..3ab292f2e 100644</p><p class="MsoNormal_mr_css_attr">> --- a/src/box/memtx_engine.c</p><p class="MsoNormal_mr_css_attr">> +++ b/src/box/memtx_engine.c</p><p class="MsoNormal_mr_css_attr">> @@ -1116,6 +1116,14 @@ memtx_engine_new(const char *snap_dirname, bool force_recovery,</p><p class="MsoNormal_mr_css_attr">> if (objsize_min < OBJSIZE_MIN)</p><p class="MsoNormal_mr_css_attr">> objsize_min = OBJSIZE_MIN;</p><p class="MsoNormal_mr_css_attr">> </p><p class="MsoNormal_mr_css_attr">> + if (alloc_factor > 2) {</p><p class="MsoNormal_mr_css_attr">> + say_error("Alloc factor must be less than or equal to 2.0. It will be reduced to 2.0");</p><p class="MsoNormal_mr_css_attr">> + alloc_factor = 2.0;</p><p class="MsoNormal_mr_css_attr">> + } else if (alloc_factor <= 1.0) {</p><p class="MsoNormal_mr_css_attr">> + say_error("Alloc factor must be greater than then 1.0. It will be increased to 1.001");</p><p class="MsoNormal_mr_css_attr"> </p><p class="MsoNormal_mr_css_attr">1. Can't parse 'than then' - please, rephrase. I assume 'then' is</p><p class="MsoNormal_mr_css_attr">not needed.</p><div class="MsoNormal_mr_css_attr"> </div><div class="MsoNormal_mr_css_attr">Fixed incorrect message, now:</div><div class="MsoNormal_mr_css_attr"> <br>+ if (alloc_factor > 2) {<br>+ say_error("Alloc factor must be less than or equal to 2.0. It will be reduced to 2.0");<br>+ alloc_factor = 2.0;<br>+ } else if (alloc_factor <= 1.0) {<br>+ say_error("Alloc factor must be greater than 1.0. It will be increased to 1.001");<br>+ alloc_factor = 1.001;<br>+ }<br>+</div><p class="MsoNormal_mr_css_attr"> </p><p class="MsoNormal_mr_css_attr">> + alloc_factor = 1.001;</p><p class="MsoNormal_mr_css_attr"> </p><p class="MsoNormal_mr_css_attr">2. I don't see the factor being rounded up in the original code.</p><p class="MsoNormal_mr_css_attr">Why did you add it? And why 1.0 is bad?</p><div class="MsoNormal_mr_css_attr"> </div><div class="MsoNormal_mr_css_attr">Answer:</div><div class="MsoNormal_mr_css_attr"> </div><div class="MsoNormal_mr_css_attr"><div>Here is a code which rounded up alloc factor in master branch.<br> /*<br> * Correct the user-supplied alloc_factor to ensure that<br> * it actually produces growing object sizes.<br> */<br> if (alloc->step_pool_objsize_max * alloc_factor <<br> alloc->step_pool_objsize_max + STEP_SIZE) {</div><div> alloc_factor =<br> (alloc->step_pool_objsize_max + STEP_SIZE + 0.5)/<br> alloc->step_pool_objsize_max;<br> }<br> I use gdb to show it:<br> tarantool> box.cfg{slab_alloc_factor = 1.01}<br> 2020-12-24 19:22:33.416 [22602] main/103/interactive C> Tarantool 2.7.0-148-gd5a5754a3<br> 2020-12-24 19:22:33.416 [22602] main/103/interactive C> log level 5<br> 2020-12-24 19:22:33.416 [22602] main/103/interactive I> mapping 268435456 bytes for memtx tuple arena...</div><div> Breakpoint 1, small_alloc_create (alloc=0x555555cfe788, cache=0x555555cfe530, objsize_min=272,<br> alloc_factor=1.00999999) at /home/evgeny/MASTER/tarantool/src/lib/small/small/small.c:135<br> 135 alloc_factor =<br> (gdb) n<br> 139 alloc->factor = alloc_factor;<br> (gdb) p alloc_factor<br> $1 = 1.032197<br> (gdb)<br> 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.<br> 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.</div></div><p class="MsoNormal_mr_css_attr"> </p><p class="MsoNormal_mr_css_attr">> + }</p><p class="MsoNormal_mr_css_attr">> diff --git a/test/engine/small_alloc_factor.result b/test/engine/small_alloc_factor.result</p><p class="MsoNormal_mr_css_attr">> new file mode 100644</p><p class="MsoNormal_mr_css_attr">> index 000000000..31d0f8ffd</p><p class="MsoNormal_mr_css_attr">> --- /dev/null</p><p class="MsoNormal_mr_css_attr">> +++ b/test/engine/small_alloc_factor.result</p><p class="MsoNormal_mr_css_attr"> </p><p class="MsoNormal_mr_css_attr">3. Please, read this document, and especially the specified</p><p class="MsoNormal_mr_css_attr">section: <a href="https://github.com/tarantool/tarantool/wiki/Code-review-procedure#testing" target="_blank">https://github.com/tarantool/tarantool/wiki/Code-review-procedure#testing</a>.</p><p class="MsoNormal_mr_css_attr">The test file names should have a special pattern. Please, follow it.</p><div class="MsoNormal_mr_css_attr"> </div><div class="MsoNormal_mr_css_attr">Rename test, add necessary gh-5216 (issue number) prefix</div><div class="MsoNormal_mr_css_attr"><div>diff --git a/test/engine/gh-5216-small_alloc_factor.lua b/test/engine/gh-5216-small_alloc_factor.lua<br>+#!/usr/bin/env tarantool<br>+<br>+require('console').listen(os.getenv('ADMIN'))<br>+<br>+box.cfg({<br>+ wal_mode = 'none',<br>+ memtx_memory = 2*1024*1024*1024,<br>+ listen = os.getenv("LISTEN"),<br>+ memtx_min_tuple_size=32,<br>+ slab_alloc_factor=1.03<br>+})<br>diff --git a/test/engine/gh-5216-small_alloc_factor.result b/test/engine/gh-5216-small_alloc_factor.result<br>+env = require('test_run')<br>+---<br>+...<br>+test_run = env.new()<br>+---<br>+...<br>+test_run:cmd('create server test with script="engine/gh-5216-small_alloc_factor.lua"')<br>+---<br>+- true<br>+...<br>+test_run:cmd('start server test with args="none"')<br>+---<br>+- true<br>+...<br>+test_run:cmd('switch test')<br>+---<br>+- true<br>+...<br>+s = box.schema.space.create('test')<br>+---<br>+...<br>+_ = s:create_index('test')<br>+---<br>+...<br>+function str(i) return string.rep('a', math.floor(256 * math.pow(1.03, i))) end<br>+---<br>+...<br>+for i=1,276 do _ = s:replace{i+200, str(i)} end<br>+---<br>+...<br>+for i=1,274 do _ = s:delete{i+200} end<br>+---<br>+...<br>+collectgarbage('collect')<br>+---<br>+- 0<br>+...<br>+_ = s:replace{200+277, str(275)}<br>+---<br>+...<br>+_ = s:delete{200+275}<br>+---<br>+...<br>+collectgarbage('collect')<br>+---<br>+- 0<br>+...<br>+_ = s:delete{200+276}<br>+---<br>+...<br>+collectgarbage('collect')<br>+---<br>+- 0<br>+...<br>+test_run:cmd('switch default')<br>+---<br>+- true<br>+...<br>+test_run:cmd('stop server test')<br>+---<br>+- true<br>+...<br>+test_run:cmd('cleanup server test')<br>+---<br>+- true<br>+...<br>+test_run:cmd('delete server test')<br>+---<br>+- true<br>+...<br>diff --git a/test/engine/gh-5216-small_alloc_factor.test.lua b/test/engine/gh-5216-small_alloc_factor.test.lua<br>+env = require('test_run')<br>+test_run = env.new()<br>+test_run:cmd('create server test with script="engine/gh-5216-small_alloc_factor.lua"')<br>+test_run:cmd('start server test with args="none"')<br>+test_run:cmd('switch test')<br>+s = box.schema.space.create('test')<br>+_ = s:create_index('test')<br>+function str(i) return string.rep('a', math.floor(256 * math.pow(1.03, i))) end<br>+for i=1,276 do _ = s:replace{i+200, str(i)} end<br>+for i=1,274 do _ = s:delete{i+200} end<br>+collectgarbage('collect')<br>+_ = s:replace{200+277, str(275)}<br>+_ = s:delete{200+275}<br>+collectgarbage('collect')<br>+_ = s:delete{200+276}<br>+collectgarbage('collect')<br>+test_run:cmd('switch default')<br>+test_run:cmd('stop server test')<br>+test_run:cmd('cleanup server test')<br>+test_run:cmd('delete server test')</div><div><br> </div></div><p class="MsoNormal_mr_css_attr"> </p></div></div></div></div></div></div><div> </div></BODY></HTML>