Re: [Tarantool-patches] [PATCH 0/3] Change small allocator behavior

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

I forward this email with some big lines truncated,
because it seems they broke our mail server.

-------- Forwarded Message --------
Subject: Re: [Tarantool-patches] [PATCH 0/3] Change small allocator behavior
Date: Sun, 20 Dec 2020 19:30:22 +0100
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: mechanik20051988 <mechanik20.05.1988@gmail.com>, tarantool-patches@dev.tarantool.org

Hi!

Thanks for the patchset!

On 18.12.2020 14:35, mechanik20051988 via Tarantool-patches wrote:
> Branch: mechanik20051988/gh-5216-fix-strange-allocator-behavior

The branch fails CI:
https://gitlab.com/tarantool/tarantool/-/pipelines/231416323

> Issue: https://github.com/tarantool/tarantool/issues/5216
> Description: Now we allocate pools on the stage of allocator creation. 
> 
> *** BLURB HERE ***

You need to replace this placeholder with your description.

I opened the branch mechanik20051988/gh-5216-fix-strange-allocator-behavior,
and I see that you didn't send the commit on top of this branch. So
I paste it below along with a review.

See 5 comments below.

>     Change small allocator behavior, add test for #5216 issue

1. Please, read https://github.com/tarantool/tarantool/wiki/Code-review-procedure
carefully. You need to use subsystem name prefix for the commit
title. Here I assume it should be 'memtx: <...>'.

>     Add a check that the slab_alloc_factor is in the range
>     (1.0, 2.0] and if it is not so changed it to 1.0001 or 2.0
>     respectively

2. Please, at least in the commit messages try to use punctuation.
Put dot in the end of sentences.

Also I guess you mean 'changed' -> 'change'.

Besides, I don't understand why is there such a restriction. Why
slab_alloc_factor 3 is bad, for example? I don't see a word about
that in the comments, in the commit message, nor in the issue
description on github (where the factor range is not even mentioned).

In addition, I don't see how 5216 is related to the factor borders.

>     Closes #5216
> 
> diff --git a/test/engine/small_alloc_factor.lua b/test/engine/small_alloc_factor.lua
> new file mode 100644
> index 000000000..b1089650e
> --- /dev/null
> +++ b/test/engine/small_alloc_factor.lua
> @@ -0,0 +1,11 @@
> +#!/usr/bin/env tarantool
> +
> +require('console').listen(os.getenv('ADMIN'))
> +
> +box.cfg({ 
> +    wal_mode = 'none', 

3. This config has trailing whitespaces. Git diff highlights
them with red color. Please, remove them.

> +    memtx_memory = 2*1024*1024*1024, 

4. Do you really need so much memory?

> +    listen = os.getenv("LISTEN"),
> +    memtx_min_tuple_size=32, 
> +    slab_alloc_factor=1.03
> +})
> diff --git a/test/engine/small_alloc_factor.result b/test/engine/small_alloc_factor.result
> new file mode 100644
> index 000000000..251a62bee
> --- /dev/null
> +++ b/test/engine/small_alloc_factor.result
> @@ -0,0 +1,73 @@
> +env = require('test_run')
> +---
> +...
> +test_run = env.new()
> +---
> +...
> +test_run:cmd('create server test with script="engine/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)}
> +---
> +- [477, 'aaaaaaaaaaaaaa...
> +...

5. The line is so huge that my email client lags. Git diff
too. Please, don't print it. The same for the other long
tuples. You are not supposed to test the console here.

Also I can't build the branch:

/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:224:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, small_table_title);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:224:19: note: treat the string as an argument to avoid this
          fprintf(stderr, small_table_title);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:225:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, underline_title_2);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:225:19: note: treat the string as an argument to avoid this
                fprintf(stderr, underline_title_2);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:226:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, small_table_title_1);
                                ^~~~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:226:19: note: treat the string as an argument to avoid this
                fprintf(stderr, small_table_title_1);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:227:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, underline_title_3);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:227:19: note: treat the string as an argument to avoid this
                fprintf(stderr, underline_title_3);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:257:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, underline_title_3);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:257:19: note: treat the string as an argument to avoid this
                fprintf(stderr, underline_title_3);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:258:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, small_table_title_3);
                                ^~~~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:258:19: note: treat the string as an argument to avoid this
                fprintf(stderr, small_table_title_3);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:259:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, underline_title_2);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:259:19: note: treat the string as an argument to avoid this
                fprintf(stderr, underline_title_2);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:260:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, small_table_title_1);
                                ^~~~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:260:19: note: treat the string as an argument to avoid this
                fprintf(stderr, small_table_title_1);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:261:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, underline_title_3);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:261:19: note: treat the string as an argument to avoid this
                fprintf(stderr, underline_title_3);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:285:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, underline_title_2);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:285:19: note: treat the string as an argument to avoid this
                fprintf(stderr, underline_title_2);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:300:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, large_table_title);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:300:19: note: treat the string as an argument to avoid this
                fprintf(stderr, large_table_title);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:301:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, underline_title_2);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:301:19: note: treat the string as an argument to avoid this
                fprintf(stderr, underline_title_2);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:302:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, small_table_title_1);
                                ^~~~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:302:19: note: treat the string as an argument to avoid this
                fprintf(stderr, small_table_title_1);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:303:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, underline_title_3);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:303:19: note: treat the string as an argument to avoid this
                fprintf(stderr, underline_title_3);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:324:19: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                fprintf(stderr, underline_title_2);
                                ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:324:19: note: treat the string as an argument to avoid this
                fprintf(stderr, underline_title_2);
                                ^
                                "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:347:20: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                        fprintf(stderr, underline_title_1);
                                        ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:347:20: note: treat the string as an argument to avoid this
                        fprintf(stderr, underline_title_1);
                                        ^
                                        "%s", 
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:349:20: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                        fprintf(stderr, underline_title_2);
                                        ^~~~~~~~~~~~~~~~~
/Users/gerold/Work/Repositories/tarantool/src/lib/small/perf/small_alloc_perf.c:349:20: note: treat the string as an argument to avoid this
                        fprintf(stderr, underline_title_2);