<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey,</p>
<p>thanks for the comments!<br>
</p>
<div class="moz-cite-prefix">On 10.02.2025 17:51, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On 04.02.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
<p>Also, sysprof has options that have no meaning for memprof:</p>
<p>mode and interval, and sysprof accepts a table when memprof
accepts</p>
<p>a string. It is not considered as a problem for you, but specific
messages</p>
<p>is a problem. Seems there is an inconsistency.</p>
<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">Plus, the error message is a little
bit incompatible with the previous version (you may notice this in test
changes).</pre>
</blockquote>
<p>We discussed this verbally (you, Sergos and me) and decided that
it is</p>
<p>not a problem.<span style="white-space: pre-wrap">
</span></p>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
<p>Honestly, it becomes more complicated in code, because error
messages are not error messages,</p>
<p>but error prefixes, and now you should care about two things:
error message and details.</p>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
Anyway, I agree that introducing error details is a compromise in a
current situtation.<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
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()`).</pre>
</blockquote>
Merged.<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
It is not reproduced with updated patchset.<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">---
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"
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Why is the default mode C and not D?
</pre>
</blockquote>
Fixed. (I remember you told me that would be good to set "C" as
default mode)<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> #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')
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<p><br>
</p>
<p>Fixed by checking a string len:</p>
<p> }<br>
mode = strVdata(mode_opt);<br>
- if (mode[1] != '\0') {<br>
+ if (strlen(mode) > 0 && mode[1] != '\0') {<br>
*err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);<br>
return PROFILE_ERRUSE;<br>
}<br>
(1/1) Stage this h<br>
</p>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
Removed parse_options.<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> {
- if (lua_gettop(L) != 1)
- return PROFILE_ERRUSE;
+ if (lua_gettop(L) != 1) {
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
<p>Good advice! Thanks, fixed.</p>
<p>@@ -180,8 +180,11 @@ static int set_output_path(const char *path,
struct luam_Sysprof_Options *opt) {<br>
<br>
static int parse_sysprof_opts(lua_State *L, struct
luam_Sysprof_Options *opt,<br>
const char **err_details) {<br>
- if (lua_gettop(L) != 1) {<br>
- lua_createtable(L, 0, 0);<br>
+ int n = (int)(L->top - L->base);<br>
+ if (n != 1) {<br>
+ opt->mode = LUAM_SYSPROF_DEFAULT;<br>
+ opt->interval = SYSPROF_DEFAULT_INTERVAL;<br>
+ goto set_path;<br>
}<br>
<br>
if (!lua_istable(L, 1)) {<br>
@@ -241,6 +244,8 @@ static int parse_sysprof_opts(lua_State *L,
struct luam_Sysprof_Options *opt,<br>
}<br>
}<br>
<br>
+set_path:<br>
+<br>
/* Get output path. */<br>
if (opt->mode != LUAM_SYSPROF_DEFAULT)<br>
{<br>
</p>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
Fixed, thanks!<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
The errors are sysprof-specific, so they should be defined under the
corresponding ifdef. The comment is irrelevant if you consider my
suggestion above.</pre>
</blockquote>
<p>Fixed.</p>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> #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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
these additional err codes were removed<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
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")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/error/no error/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+test:is(errno, nil, "errno with missed profiling mode")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/errno/no errno/
</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+assert(misc.sysprof.stop(), "sysprof is not stopped")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I would rather drop the "sysprof is not stopped" -- in that case we will
see the reason _why_ it is not stopped successfully instead.</pre>
</blockquote>
Removed.<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+-- 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{
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6oSjkBsE2C0fbUl@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>