From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C629F4765E0 for ; Mon, 21 Dec 2020 14:38:54 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <8d42b447-057b-671c-c621-d432a1c365e9@tarantool.org> Date: Mon, 21 Dec 2020 12:38:52 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 0/3] Change small allocator behavior List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mechanik20051988 , tarantool-patches@dev.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 To: mechanik20051988 , 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);