[Tarantool-patches] FW: [PATCH] memtx: change small allocator behavior
Evgeny Mekhanik
mechanik20051988 at tarantool.org
Fri Dec 25 10:42:44 MSK 2020
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')
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20201225/6c1ffed5/attachment.html>
More information about the Tarantool-patches
mailing list