Tarantool development patches archive
 help / color / mirror / Atom feed
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 --]

  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