From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 9BB6D57B70D; Wed, 19 Feb 2025 18:20:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9BB6D57B70D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1739978455; bh=/t1GnWg28q2FA+E8dlIxeWkE5SPZGw13JqL6h46Qnys=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=jJ5iFV2QD629PQMyp9TQN9CpRJLoA1iajIzhRjbYv/oZ3KOfIWlLjEK13SiEUWrAD g1ucjh0G4hnIGn+rgPZ/dwdja7FkRSnCVzf+zOkSFhZTzVe8+y9L0X5PdZQFdqeP/y lLj/Q4G1N0ZisQIG5ZsXzCYpPypUDFdxmNWZ3w+Q= Received: from send195.i.mail.ru (send195.i.mail.ru [95.163.59.34]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9DBBF57B70D for ; Wed, 19 Feb 2025 18:20:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9DBBF57B70D Received: by exim-smtp-5f589487f8-2j7d5 with esmtpa (envelope-from ) id 1tklsa-00000000H7M-2gLm; Wed, 19 Feb 2025 18:20:53 +0300 Date: Wed, 19 Feb 2025 18:20:10 +0300 To: Sergey Bronnikov Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD916C41472748AFA04A64163DEC989407BAC72CC1C2A047AB700894C459B0CD1B99A634466FBAE92BF89CEA9C911D09995379A69B214AD591B49ACF7309BA9B08583F5067A33008722 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70CB15FA6C489297DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637663000D94094A5D68638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8AEA4A44BDA680346A31576F665564879B061B188ED53A5ADCC7F00164DA146DAFE8445B8C89999728AA50765F7900637CAEE156C82D3D7D9389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8989FD0BDF65E50FBF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C13BDA61BF53F5E1D03F1AB874ED890284AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3905409F363357F83BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE74ABCC139FF3F849B731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A55FC8FD61FE213A185002B1117B3ED6963EC32EA5B620797B33EE06AFCD964888823CB91A9FED034534781492E4B8EEADEEA082C9A12FE455BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D342C0B628602DFD0BCDAA53AA4DD24C48BCC6DD1042841285EED75B0379B52EC23667A2822486BF6121D7E09C32AA3244CFF1533E2F86679A777DD89D51EBB7742877D149E0D97EEF9EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59829709634694AABAED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVWiyXSWEEqdrpF07QyQIJbA= X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B59323EA4370A5B050C3DE06ABAFEAF67059E8A153705F7B294B7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 4/7][v2] sysprof: introduce specific errors and default mode X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the fixes! Adds some comments related to our offline discussion below. On 19.02.25, Sergey Bronnikov wrote: > Hi, Sergey, > > thanks for review! > > On 18.02.2025 18:43, Sergey Kaplun via Tarantool-patches wrote: > > Hi, Sergey! > > Thanks for the fixes! > > After refactoring the code become more readable, thanks! > > Now I have a few ideas, see my comments below. > > > > On 13.02.25, Sergey Bronnikov wrote: > >> sysprof has a number of options and with any incorrect option it > >> returns `false` and error message "profiler misuse". This message > >> discourage sysprof users and make using sysprof more complicated. > > Typo: s/discourage/discourages/ > > Typo: s/make/makes/ > Fixed. > > > >> The patch sets default profiling mode ("D", that shows only > > Typo: s/default/the default/ > Fixed. > > > >> virtual machine state counters) if it was not passed and adds > >> details to the error message with possible reasons of misuse. > >> --- > >> src/lib_misc.c | 80 +++++++++++++------ > >> src/lj_errmsg.h | 5 ++ > >> .../profilers/misclib-sysprof-lapi.test.lua | 48 +++++++++-- > >> 3 files changed, 100 insertions(+), 33 deletions(-) > >> > >> diff --git a/src/lib_misc.c b/src/lib_misc.c > >> index 5b7a4b62..d71904e4 100644 > >> --- a/src/lib_misc.c > >> +++ b/src/lib_misc.c > >> @@ -163,6 +163,7 @@ static int on_stop_cb_default(void *opt, uint8_t *buf) > >> > >> /* The default profiling interval equals to 10 ms. */ > >> #define SYSPROF_DEFAULT_INTERVAL 10 > >> +#define SYSPROF_DEFAULT_MODE "D" > >> #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin" > >> > >> static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) { > >> @@ -177,21 +178,41 @@ static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) { > >> return PROFILE_SUCCESS; > >> } > >> > >> -static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, int idx) { > >> - GCtab *options = lj_lib_checktab(L, idx); > >> +static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, > >> + const char **err_details) { > >> + int n = (int)(L->top - L->base); > >> + if (n != 1) { > > I suppose this should be `n == 0`. Otherwise we will observe the > > following behaviour: > > | src/luajit -e 'print(misc.sysprof.start(1, 2, 3))' > > | true > Right, fixed. > >> + opt->mode = LUAM_SYSPROF_DEFAULT; > >> + opt->interval = SYSPROF_DEFAULT_INTERVAL; > >> + goto set_path; > > I suppose it is better to set path explicitly here and goto ctx_allocate; > > The code that make a jump to the middle of basic block smells bad. > > It makes control flow more complicated, and benefits are not obvious. I suppose we may just return here since we don't get into the if condition under the label below. > > > > >> + } > >> + > >> + if (!lua_istable(L, 1)) { > > The more I think about it, the more it looks like a bug -- all library > > functions raise an error when they get a bad parameter type. > > > > Maybe we should just use `lj_lib_checktab()` here? This will help the > > user to find an error and be consistent with memprof behaviour. So, > > bugfix isn't breaking change, since something is working incorrectly. > > Plus, as a bonus we don't need to introduce the new error with the same > > meaning we have already. > > > > OTOH, if you are against it, we may leave it as is, but check it via > > `tvistab()` instead. > > > > What do you think? > > > > And the same should be done with the parameters in table content -- > > their type should be checked at once and the corresponding error should > > be raised. I suppose we can introduce a local (inside this translation > > unit) helper `key_opt_type()` here -- we will check a type for the keys > > `path`, `mode`, `interval` and raise the user-friendly error like: > > What for? The reason of this patch series is bad error handling in > profilers. > > The value behind this refactoring is not obvious. Please elaborate. > > This code was imperfect for about 2-3 years and this was ok for everyone > > who added it initially. I don't want to fix all bad places here, just > improve error messages visible by end users. Discussed offline only to fix the bug with `lua_istable()` vs `lj_lib_checktab()`. The options validation is OK it is the current state. > > > > > `bad key 'mode' in 'start' argument (string expected, got table)`. > > See how it is done in `lj_lib_check*()` for inspiration. > > > >> + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADTABLE); > >> + return PROFILE_ERRUSE; > >> + } > >> + > >> + GCtab *options = lj_lib_checktab(L, 1); > > This check is excess, let's move it above, as I suggested. > > it is not a check, it's a function that retrieves a table from the stack. See the comment above. > >> +set_path: > >> + > >> /* Get output path. */ > >> if (opt->mode != LUAM_SYSPROF_DEFAULT) > >> { > >> @@ -230,8 +256,10 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in > >> cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path")); > >> if (!pathtv) > >> path = SYSPROF_DEFAULT_OUTPUT; > >> - else if (!tvisstr(pathtv)) > >> + else if (!tvisstr(pathtv)) { > > It is better to wrap if and else branches too like the following: > > | if () { > > | } else if () { > > | } else { > > | } > > It's funny, on the previous review brackets were excess [1]. > > 1. https://lists.tarantool.org/tarantool-patches/Z6XlUsKqu92__8fL@root/T/#t This was about single if usage not `if-else if-else` statements. > > Brackets are not needed in the first and third blocks, if we're > following a rule "brackets for blocks with more than one statements". > > > >> + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH); > >> return PROFILE_ERRUSE; > >> + } > >> else > >> path = strVdata(pathtv); > >> > >> @@ -251,29 +279,28 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in > >> return PROFILE_SUCCESS; > >> } > >> > >> -static int parse_options(lua_State *L, struct luam_Sysprof_Options *opt) > >> -{ > >> - if (lua_gettop(L) != 1) > >> - return PROFILE_ERRUSE; > >> - > >> - if (!lua_istable(L, 1)) > >> - return PROFILE_ERRUSE; > >> - > >> - return parse_sysprof_opts(L, opt, 1); > >> -} > >> - > >> -static int sysprof_error(lua_State *L, int status) > >> +static int sysprof_error(lua_State *L, int status, const char *err_details) > >> { > >> switch (status) { > >> case PROFILE_ERRUSE: > >> lua_pushnil(L); > >> lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); > >> + if (err_details) { > >> + lua_pushstring(L, ": "); > > I suppose we may use string formatting here instead of concatenation Lua > > C API call: > > See usage of `lj_strfmt_pushf()` for the details. > > Sure, we may, but what for? > > Previous functions are a part of Lua C API (lua_pushnil, lua_pushstring), > > why do we need to mix Lua C functions with this LJ-specific function? We don't bother the Lua stack with excess pushes and additional concatenation, if we know exactly how many values on the stack we need. > >> diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h > >> index 19c41f0b..b5c3a275 100644 > >> --- a/src/lj_errmsg.h > >> +++ b/src/lj_errmsg.h > >> @@ -188,6 +188,11 @@ ERRDEF(PROF_ISRUNNING, "profiler is running already") > >> ERRDEF(PROF_NOTRUNNING, "profiler is not running") > >> #endif > >> > >> +ERRDEF(PROF_DETAILS_BADMODE, "profiler 'mode' must be 'D', 'L' or 'C'") > >> +ERRDEF(PROF_DETAILS_BADINTERVAL, "profiler 'interval' must be greater than 1") > >> +ERRDEF(PROF_DETAILS_BADPATH, "profiler path does not exist") This should be something like the following: | "profiler 'path' should be a string" > >> +ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters") > >> + > > These changes should be under the LJ_HASSYSPROF ifdef. > > Also, I suggest the following naming: > > | SYSPROF_BADMODE > > | SYSPROF_BADINTERVAL > > With this it will be obviour that they are sysprof-specific and not too > > long. > > Imagine you then introduce a table and interval number for memprof. > > Will you rename constants back? I dislike your suggestion. OK, let's leave it as is. > > > > >> #undef ERRDEF > >> > >> /* Detecting unused error messages: > >> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua > >> index 32fa384c..7622323a 100644 > >> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua > >> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua > >> @@ -10,7 +10,7 @@ local test = tap.test("misclib-sysprof-lapi"):skipcond({ > >> ["Disabled due to #10803"] = os.getenv("LUAJIT_TEST_USE_VALGRIND"), > >> }) > >> > >> -test:plan(19) > >> +test:plan(33) > >> > >> jit.off() > >> -- XXX: Run JIT tuning functions in a safe frame to avoid errors > >> @@ -65,10 +65,25 @@ end > >> > >> -- Wrong profiling mode. > >> local res, err, errno = misc.sysprof.start{ mode = "A" } > >> -test:ok(res == nil anderr:match("profiler misuse"), > >> - "result status with wrong profiling mode") > >> +test:ok(res == nil, "result status with wrong profiling mode") > >> +test:ok(err:match("profiler mode must be 'D', 'L' or 'C'"), > >> + "error with wrong profiling mode") > > I would rather still check matching with `profiler misuse:` error (as a > > separate testcase). Here and below. But it's up to you. Feel free to > > ignore. > I think that checking specific part of error message is enough. Ok. > >> @@ -92,11 +107,30 @@test:ok(res == nil anderr:match("No such file or directory"), > >> "result status and error with bad path") > >> test:ok(type(errno) == "number", "errno with bad path") > >> > >> --- Bad interval. > >> +-- Bad interval (-1). > >> res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 } > >> -test:ok(res == nil anderr:match("profiler misuse"), > >> - "result status and error with bad interval") > >> -test:ok(type(errno) == "number", "errno with bad interval") > >> +test:is(res, nil, "result status and error with bad interval -1") > >> +test:ok(err:match("profiler interval must be greater than 1"), > >> + "error with bad interval -1") > >> +test:ok(type(errno) == "number", "errno with bad interval -1") > >> + > >> +-- Bad interval (0). > >> +res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 } > >> +test:ok(res == nil, "res with bad interval 0") > >> +test:ok(err:match("profiler interval must be greater than 1"), > >> + "error with bad interval 0") > >> +test:ok(type(errno) == "number", "errno with bad interval 0") > >> + > >> +-- Good interval (1). > >> +res, err, errno = misc.sysprof.start{ > > Minor: Please use brackets for functions call. > > Case without brackets is more popular: > proposed change will make coding style inconsistent in the file OK, my bad. > > > > >> -- > >> 2.34.1 > >> -- Best regards, Sergey Kaplun