[Tarantool-patches] [PATCH luajit 3/4] sysprof: introduce specific errors and default mode
Sergey Kaplun
skaplun at tarantool.org
Mon Feb 10 17:51:58 MSK 2025
Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On 04.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
> does not descriptive for sysprof users and make using sysprof
> more complicated. The patch splits error `PROFILE_ERRUSE` to four
> specific errors: `PROFILE_ERRBADMODE`, `PROFILE_ERRBADINTERVAL`,
> `PROFILE_ERRBADPATH` and `PROFILE_ERRBADTABLE`, and use default
> profiling mode ("C", callgraph) if it was not passed.
I don't like this approach:
Now we have profiler-specific statuses (valid only for sysprof) in the
public API. They have no meaning for memprof. Also, that makes the
definition of errors more twisted. Plus, the error message is a little
bit incompatible with the previous version (you may notice this in test
changes).
I suggest the following approach:
We append all additional details in the error message, so it looks
like:
| profiler misuse: sysprof mode must be 'D', 'L' or 'C'
The implementation is also simple. We don't need any additional statuses
for error messages, just to return in the `char **` pointer to string as
additional details.
So the usage is the following:
`parse_sysprof_opts(L, &opts, &err_details);`
`sysprof_error(L, &opts, err_details); /*details may be NULL here */`
Pros:
* No custom statuses for sysprof in the public API (or reserved for
internal usage).
* More backward compatible with previous errors -- error message
still matches with 'profiler misuse'.
* No additional custom error definitions.
* More consistent behaviour between Lua and C API -- as we operate only
`PROFILER_MISUSE` status everywhere.
* It simplifies error handling in `sysprof_error()` -- we just need to
check if the given argument is NULL.
Cons:
* Additional argument to the internal functions `sysprof_error()`,
`parse_sysprof_opts()`.
Side note: I believe, since we have no valuable logic in the
`parse_opts()`, its body can be merged in the `parse_sysprof_opts()`).
See my comment below.
Friendly reminder: Also, I got the following error when building LuaJIT
with sysprof disabled (we discussed it online):
| src/luajit -e 'misc.sysprof.start()'
| LuaJIT ASSERT /home/burii/reviews/luajit/sysprof-fixes/src/lj_state.c:214: close_state: memory leak of 8388624 bytes
| Aborted (core dumped)
It seems that this is an old problem unrelated to your patchset. This
happens for a non-default mode if we use the build with disabled
sysprof. Looks like we still allocated the write buffer for sysprof for
some reason.
> ---
> src/lib_misc.c | 47 ++++++++++++++-----
> src/lj_errmsg.h | 4 ++
> src/lmisclib.h | 5 ++
> .../profilers/misclib-sysprof-lapi.test.lua | 45 ++++++++++++++++--
> 4 files changed, 85 insertions(+), 16 deletions(-)
>
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index 5b7a4b62..a28e4a3c 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 "C"
Why is the default mode C and not D?
> #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin"
>
> static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) {
> @@ -185,13 +186,16 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
> const char *mode = NULL;
>
> cTValue *mode_opt = lj_tab_getstr(options, lj_str_newlit(L, "mode"));
> - if (!mode_opt || !tvisstr(mode_opt)) {
> - return PROFILE_ERRUSE;
> + if (mode_opt) {
> + if (!tvisstr(mode_opt))
> + return PROFILE_ERRBADMODE;
> + mode = strVdata(mode_opt);
> + if (mode[1] != '\0')
If we have `mode = ''`, this will read the byte from `g->hookmask`. The
result is always `PROFILER_ERRBADMODE` (in the current terms), but it is
still dirty reads.
> + return PROFILE_ERRBADMODE;
> }
>
> - mode = strVdata(mode_opt);
> - if (mode[1] != '\0')
> - return PROFILE_ERRUSE;
> + if (!mode)
> + mode = SYSPROF_DEFAULT_MODE;
>
> switch (*mode) {
> case 'D':
> @@ -204,7 +208,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
> opt->mode = LUAM_SYSPROF_CALLGRAPH;
> break;
> default:
> - return PROFILE_ERRUSE;
> + return PROFILE_ERRBADMODE;
> }
> }
>
> @@ -215,7 +219,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
> if (interval && tvisnumber(interval)) {
> int32_t signed_interval = numberVint(interval);
> if (signed_interval < 1)
> - return PROFILE_ERRUSE;
> + return PROFILE_ERRBADINTERVAL;
> opt->interval = signed_interval;
> }
> }
> @@ -231,7 +235,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
> if (!pathtv)
> path = SYSPROF_DEFAULT_OUTPUT;
> else if (!tvisstr(pathtv))
> - return PROFILE_ERRUSE;
> + return PROFILE_ERRBADPATH;
> else
> path = strVdata(pathtv);
>
> @@ -253,11 +257,12 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>
> static int parse_options(lua_State *L, struct luam_Sysprof_Options *opt)
I suppose this helper function isn't needed. It is not related to
memprof, so its name is misleading. Its body may be merged with
`parse_sysprof_options()` to avoid these problems.
> {
> - if (lua_gettop(L) != 1)
> - return PROFILE_ERRUSE;
> + if (lua_gettop(L) != 1) {
If there are 2+ arguments with different content, we still create a
table. I suppose there is no need for table creation. We may just set
the default options at once without additional processing and creation
of the table.
Minor: Also, maybe it is better to use `(int)(L->top - L->base)` instead
of the call to the `lua_gettop()` for consistency with the internal
LuaJIT code base. I know that this function does the same thing, but
since it is the public API, this call isn't inlined.
> + lua_createtable(L, 0, 0);
> + }
>
> if (!lua_istable(L, 1))
> - return PROFILE_ERRUSE;
> + return PROFILE_ERRBADTABLE;
>
> return parse_sysprof_opts(L, opt, 1);
> }
> @@ -270,6 +275,26 @@ static int sysprof_error(lua_State *L, int status)
> lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
> lua_pushinteger(L, EINVAL);
> return 3;
> + case PROFILE_ERRBADMODE:
> + lua_pushnil(L);
> + lua_pushstring(L, err2msg(LJ_ERR_PROF_BADMODE));
> + lua_pushinteger(L, EINVAL);
> + return 3;
> + case PROFILE_ERRBADINTERVAL:
> + lua_pushnil(L);
> + lua_pushstring(L, err2msg(LJ_ERR_PROF_BADINTERVAL));
> + lua_pushinteger(L, EINVAL);
> + return 3;
> + case PROFILE_ERRBADPATH:
> + lua_pushnil(L);
> + lua_pushstring(L, err2msg(LJ_ERR_PROF_BADPATH));
> + lua_pushinteger(L, EINVAL);
> + return 3;
> + case PROFILE_ERRBADTABLE:
> + lua_pushnil(L);
> + lua_pushstring(L, err2msg(LJ_ERR_PROF_BADTABLE));
> + lua_pushinteger(L, EINVAL);
> + return 3;
> #if LJ_HASSYSPROF
> case PROFILE_ERRRUN:
> lua_pushnil(L);
> diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h
> index 19c41f0b..9713d6c7 100644
> --- a/src/lj_errmsg.h
> +++ b/src/lj_errmsg.h
> @@ -183,6 +183,10 @@ ERRDEF(FFI_NYICALL, "NYI: cannot call this C function (yet)")
>
> /* Profiler errors. */
> ERRDEF(PROF_MISUSE, "profiler misuse")
> +ERRDEF(PROF_BADMODE, "profiler mode must be 'D', 'L' or 'C'")
> +ERRDEF(PROF_BADINTERVAL, "profiler interval must be greater than 1")
> +ERRDEF(PROF_BADPATH, "profiler path does not exist")
> +ERRDEF(PROF_BADTABLE, "profiler expects a table with parameters")
The errors are sysprof-specific, so they should be defined under the
corresponding ifdef. The comment is irrelevant if you consider my
suggestion above.
> #if LJ_HASMEMPROF || LJ_HASSYSPROF
> ERRDEF(PROF_ISRUNNING, "profiler is running already")
> ERRDEF(PROF_NOTRUNNING, "profiler is not running")
> diff --git a/src/lmisclib.h b/src/lmisclib.h
> index 9084319c..b4eb0b57 100644
> --- a/src/lmisclib.h
> +++ b/src/lmisclib.h
> @@ -142,6 +142,11 @@ struct luam_Sysprof_Options {
> #define PROFILE_ERRMEM 3
> #define PROFILE_ERRIO 4
>
> +#define PROFILE_ERRBADMODE 5
> +#define PROFILE_ERRBADINTERVAL 6
> +#define PROFILE_ERRBADPATH 7
> +#define PROFILE_ERRBADTABLE 8
The errors are sysprof-specific, so they should be defined under the
corresponding ifdef. Also, they are blocking the future introduction of
profiler-general errors. The comment is irrelevant if you consider my
suggestion above.
> +
> LUAMISC_API int luaM_sysprof_set_writer(luam_Sysprof_writer writer);
>
> LUAMISC_API int luaM_sysprof_set_on_stop(luam_Sysprof_on_stop on_stop);
> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> index 581fb7fd..68a4b72f 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("misc-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,9 +65,25 @@ end
>
> -- Wrong profiling mode.
> local res, err, errno = misc.sysprof.start{ mode = "A" }
> -test:ok(res == nil and err:match("profiler misuse"), "res with no parameters")
> +test:ok(res == nil, "res with no parameters")
> +test:ok(err:match("profiler mode must be 'D', 'L' or 'C'"),
> + "error with no parameters")
> test:ok(type(errno) == "number", "errno with no parameters")
>
> +-- Missed profiling mode.
> +res, err, errno = misc.sysprof.start{}
> +test:is(res, true, "res with missed profiling mode")
> +test:is(err, nil, "error with missed profiling mode")
Typo: s/error/no error/
> +test:is(errno, nil, "errno with missed profiling mode")
Typo: s/errno/no errno/
> +assert(misc.sysprof.stop(), "sysprof is not stopped")
I would rather drop the "sysprof is not stopped" -- in that case we will
see the reason _why_ it is not stopped successfully instead.
> +
> +-- Not a table.
> +res, err, errno = misc.sysprof.start("NOT A TABLE")
> +test:ok(res == nil, "res with not a table")
> +test:ok(err:match("profiler expects a table with parameters"),
> + "error with not a table")
> +test:ok(type(errno) == "number", "errno with not a table")
> +
> -- Already running.
> res, err = misc.sysprof.start{ mode = "D" }
> assert(res, err)
> @@ -90,10 +106,29 @@ res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
> test:ok(res == nil and err:match("No such file or directory"), "res 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 and err:match("profiler misuse"), "res and error with bad interval")
> -test:ok(type(errno) == "number", "errno with bad interval")
> +test:ok(res == nil, "res 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{
Here we created a file and don't delete it after the test ends. Maybe it
is better to use '/dev/null', since we don't care about the content of
the file.
> + mode = "C",
> + interval = 1,
> +}
> +test:is(res, true, "res with good interval 1")
> +test:is(err, nil, "error with good interval 1")
> +test:is(errno, nil, "errno with good interval 1")
> +misc.sysprof.stop()
>
> -- DEFAULT MODE
>
> --
> 2.34.1
>
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list