[Tarantool-patches] [PATCH luajit 4/8][v3] sysprof: introduce specific errors and default mode
Sergey Kaplun
skaplun at tarantool.org
Mon Feb 24 15:46:04 MSK 2025
Hi, Sergey!
Thanks for the fixes!
LGTM, after the final polishing, please consider my comments below.
On 20.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
> discourages sysprof users and makes using sysprof more
> complicated. The patch sets the default profiling mode ("D", that
> shows only virtual machine state counters) if it was not passed
> and adds details to the error message with possible reasons of
> misuse.
Please add the following notes.
| Also, it fixes the bug when the error isn't raised if the given argument
| type is incorrect.
| Also, the profiler can start if it has extra arguments given.
| They are just ignored now.
> ---
> src/lib_misc.c | 77 ++++++++++++-------
> src/lj_errmsg.h | 5 ++
> .../profilers/misclib-sysprof-lapi.test.lua | 68 ++++++++++++++--
> 3 files changed, 114 insertions(+), 36 deletions(-)
>
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index 5b7a4b62..1fd06dd1 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -14,6 +14,7 @@
>
> #include "lj_obj.h"
> #include "lj_str.h"
> +#include "lj_strfmt.h"
Please add the corresponding include (after lj_str.h) in <src/Makefile.dep.original> to
avoid conflicts when we use this file (for crossbuilds or whatever).
> #include "lj_tab.h"
> #include "lj_lib.h"
> #include "lj_gc.h"
> @@ -163,6 +164,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 +179,36 @@ 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) {
Nit: Please, replace every 8 spaces with one tab.
> + int n = (int)(L->top - L->base);
> + if (n == 0) {
> + opt->mode = LUAM_SYSPROF_DEFAULT;
> + opt->interval = SYSPROF_DEFAULT_INTERVAL;
> + return PROFILE_SUCCESS;
> + }
> +
> + GCtab *options = lj_lib_checktab(L, 1);
Please, declare (not initialize) `options` before `if` branch to avoid
-Wdeclaration-after-statement warnings.
Please, also add the comment that we ignore all other arguments given to
this function.
>
> /* Get profiling mode. */
> {
<snipped>
> }
> @@ -214,8 +232,10 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
> opt->interval = SYSPROF_DEFAULT_INTERVAL;
> if (interval && tvisnumber(interval)) {
> int32_t signed_interval = numberVint(interval);
> - if (signed_interval < 1)
> + if (signed_interval < 1) {
> + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL);
> return PROFILE_ERRUSE;
> + }
> opt->interval = signed_interval;
> }
> }
> @@ -230,9 +250,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)
Please, add the `{` braces here too.
> path = SYSPROF_DEFAULT_OUTPUT;
> - else if (!tvisstr(pathtv))
> + else if (!tvisstr(pathtv)) {
> + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);
> return PROFILE_ERRUSE;
> - else
> + } else
Ditto.
> path = strVdata(pathtv);
>
> ctx = lj_mem_new(L, sizeof(*ctx));
> @@ -251,29 +272,26 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
> return PROFILE_SUCCESS;
> }
<snipped>
> +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) {
Nit: OTOH, there is no need for them here and below.
Feel free to ignore.
> + lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
> + } else {
> + lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
> + }
> lua_pushinteger(L, EINVAL);
> return 3;
> #if LJ_HASSYSPROF
> case PROFILE_ERRRUN:
> lua_pushnil(L);
> - lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
> + if (err_details) {
Side note: `err_deails` is always `NULL` here. I suppose we don't need
this branch for now (to avoid the dead code), so these changes may be
avoided.
> + lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
Typo: s/LJ_ERR_PROF_MISUSE/LJ_ERR_PROF_ISRUNNING/
Irrelevant, after fixing the comment above.
> + } else {
> + lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
> + }
> lua_pushinteger(L, EINVAL);
> return 3;
> case PROFILE_ERRIO:
> @@ -291,15 +309,16 @@ LJLIB_CF(misc_sysprof_start)
<snipped>
> 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")
This errmsg is unused now and can be deleted.
> +
> #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..9915f565 100644
> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
<snipped>
> @@ -24,6 +24,8 @@ local profilename = require("utils").tools.profilename
>
> local TMP_BINFILE = profilename("sysprofdata.tmp.bin")
> local BAD_PATH = profilename("sysprofdata/tmp.bin")
> +local BAD_MODE = "A"
> +local BAD_INTERVAL = -1
Why do we need this definition for only one type of bad interval?
Let's consistently declare both of them like:
| local BAD_INTERVAL_M1 = -1
| local BAD_INTERVAL_0 = 0
Or don't declare them at all.
I'm OK with both variants.
>
> local function payload()
> local function fib(n)
> @@ -64,11 +66,44 @@ end
<snipped>
> test:ok(type(errno) == "number", "errno with wrong profiling mode")
>
> +-- Missed profiling mode.
> +res, err, errno = misc.sysprof.start{}
> +test:is(res, true, "res with missed profiling mode")
> +test:is(err, nil, "no error with missed profiling mode")
> +test:is(errno, nil, "no errno with missed profiling mode")
> +assert(misc.sysprof.stop())
Minor: It would be nice to check that there is no default file created,
i.e., that we actually run the default profiler mode without stack
inspection. Feel free to ignore, through.
> +
> +-- Not a table.
> +res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE")
> +print(res, err, errno)
Excess debug print.
> +test:is(res, false, "res with not a table")
> +test:ok(err:match("table expected, got string"),
> + "error with not a table")
> +test:is(errno, nil, "errno with not a table")
There is no need to check or declare the `errno` variable. It's always
`nil` for the case when the error is raised.
> +
> +-- All parameters are invalid.
> +res, err, errno = misc.sysprof.start({
> + mode = BAD_MODE, path = BAD_PATH, interval = BAD_INTERVAL })
> +test:ok(res == nil, "res with all invalid parameters")
> +test:ok(err:match("profiler misuse: profiler mode must be 'D', 'L' or 'C'"),
> + "error with all invalid parameters")
> +test:ok(type(errno) == "number", "errno with all invalid parameters")
> +
> +-- All parameters are invalid, except the first one.
> +res, err, errno = misc.sysprof.start({
> + mode = "C", path = BAD_PATH, interval = BAD_INTERVAL })
> +test:ok(res == nil, "res with all invalid parameters except the first one")
> +test:ok(err:match("profiler misuse: profiler interval must be greater than 1"),
> + "error with all invalid parameters except the first one")
> +test:ok(type(errno) == "number",
> + "errno with all invalid parameters except the first one")
This is not exactly what I mean by invalid/partially invalid parameters
(sorry, for the bad wording):
I mean to add tests that cover the following calls:
| pcall(misc.sysprof.start, '', '', '') -- shouldn't started -- error
| -- (expected table got nil)
| misc.sysprof.start({}, '', '') -- should started with the default options
> +
> -- Already running.
> res, err = misc.sysprof.start{ mode = "D" }
> assert(res, err)
> @@ -93,11 +128,30 @@ test:ok(res == nil and err:match("No such file or directory"),
> test:ok(type(errno) == "number", "errno with bad path")
>
> -- Bad interval.
> -res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
> -test:ok(res == nil and err:match("profiler misuse"),
> - "result status and error with bad interval")
> +res, err, errno = misc.sysprof.start{ mode = "C", interval = BAD_INTERVAL }
Side note: See my comments about this constant above.
> +test:is(res, nil, "result status and error with bad interval")
> +test:ok(err:match("profiler interval must be greater than 1"),
> + "error with bad interval")
> test:ok(type(errno) == "number", "errno with bad interval")
>
> +-- 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{
> + mode = "C",
> + interval = 1,
> + path = "/dev/null",
> +}
> +test:is(res, true, "res with good interval 1")
> +test:is(err, nil, "no error with good interval 1")
> +test:is(errno, nil, "no errno with good interval 1")
> +misc.sysprof.stop()
> +
> -- DEFAULT MODE
>
> if not pcall(generate_output, { mode = "D", interval = 100 }) then
> --
> 2.43.0
>
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list