[Tarantool-patches] [PATCH luajit 0/4] Fix sysprof issues

[Tarantool-patches] [PATCH luajit 0/4] Fix sysprof issues

[Sergey Bronnikov via Tarantool-patches]

The patch series fixes a number of issues related to sysprof:

- Fixes a problem with wrong error message on stop not running
  profiler.
- Make error messages more specific and descriptive.
- Introduces default profiling mode.
- Fixes descriptions in sysprof testcases.

Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-fix-sysprof-opts-processing
(aarch64 jobs are failed, it is a known issue.)

Sergey Bronnikov (4):
  test: add descriptions to sysprof testcases
  sysprof: fix typo in the comment
  sysprof: introduce specific errors and default mode
  sysprof: fix a message with stop without run

 src/lib_misc.c                                | 49 +++++++++---
 src/lj_errmsg.h                               |  4 +
 src/lj_sysprof.c                              |  2 +-
 src/lmisclib.h                                |  5 ++
 .../profilers/misclib-sysprof-lapi.test.lua   | 76 ++++++++++++++-----
 5 files changed, 105 insertions(+), 31 deletions(-)

-- 
2.34.1

[Tarantool-patches] [PATCH luajit 1/4] test: add descriptions to sysprof testcases

[Sergey Bronnikov via Tarantool-patches]

The patch add descriptions to sysprof testcases to make TAP report
more usable.
---
 .../profilers/misclib-sysprof-lapi.test.lua   | 36 ++++++++++---------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index b2c38c49..581fb7fd 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -65,34 +65,35 @@ end
 
 -- Wrong profiling mode.
 local res, err, errno = misc.sysprof.start{ mode = "A" }
-test:ok(res == nil and err:match("profiler misuse"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("profiler misuse"), "res with no parameters")
+test:ok(type(errno) == "number", "errno with no parameters")
 
 -- Already running.
 res, err = misc.sysprof.start{ mode = "D" }
 assert(res, err)
 
 res, err, errno = misc.sysprof.start{ mode = "D" }
-test:ok(res == nil and err:match("profiler is running already"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("profiler is running already"),
+        "ok with already running")
+test:ok(type(errno) == "number", "errno with already running")
 
 res, err = misc.sysprof.stop()
 assert(res, err)
 
 -- Not running.
 res, err, errno = misc.sysprof.stop()
-test:ok(res == nil and err)
-test:ok(type(errno) == "number")
+test:ok(res == nil and err, "res and error with not running")
+test:ok(type(errno) == "number", "errno with not running")
 
 -- Bad path.
 res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
-test:ok(res == nil and err:match("No such file or directory"))
-test:ok(type(errno) == "number")
+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.
 res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
-test:ok(res == nil and err:match("profiler misuse"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("profiler misuse"), "res and error with bad interval")
+test:ok(type(errno) == "number", "errno with bad interval")
 
 -- DEFAULT MODE
 
@@ -102,20 +103,23 @@ end
 
 local report = misc.sysprof.report()
 
--- Check the profile is not empty
-test:ok(report.samples > 0)
+-- Check the profile is not empty.
+test:ok(report.samples > 0, "number of samples is greater than 0")
 -- There is a Lua function with FNEW bytecode in it. Hence there
 -- are only three possible sample types:
 -- * LFUNC -- Lua payload is sampled.
 -- * GC -- Lua GC machinery triggered in scope of FNEW bytecode
 --   is sampled.
 -- * INTERP -- VM is in a specific state when the sample is taken.
-test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0)
+test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0,
+        "total number of LFUNC, GC and INTERP samples is greater than 0")
 -- There is no fast functions and C function in default payload.
-test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0)
+test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0,
+        "total number of FFUNC and CFUNC samples is equal to 0")
 -- Check all JIT-related VM states are not sampled.
 for _, vmstate in pairs({ 'TRACE', 'RECORD', 'OPT', 'ASM', 'EXIT' }) do
-  test:ok(report.vmstate[vmstate] == 0)
+  local msg = ("total number of VM state %s is equal to 0"):format(vmstate)
+  test:ok(report.vmstate[vmstate] == 0, msg)
 end
 
 -- With very big interval.
@@ -124,7 +128,7 @@ if not pcall(generate_output, { mode = "D", interval = 1000 }) then
 end
 
 report = misc.sysprof.report()
-test:ok(report.samples == 0)
+test:ok(report.samples == 0, "total number of samples is equal to 0")
 
 -- LEAF MODE
 check_mode("L", 100)
-- 
2.34.1

[Tarantool-patches] [PATCH luajit 2/4] sysprof: fix typo in the comment

[Sergey Bronnikov via Tarantool-patches]

The patch fixes inconsistency in comment and source code below
the comment. The typo was added in the initial commit
2593a9f83072 ("misc: introduce Lua API for new sampling profiler").
---
 src/lib_misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lib_misc.c b/src/lib_misc.c
index ca1d1c75..5b7a4b62 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -161,7 +161,7 @@ static int on_stop_cb_default(void *opt, uint8_t *buf)
 
 #define LJLIB_MODULE_misc_sysprof
 
-/* The default profiling interval equals to 11 ms. */
+/* The default profiling interval equals to 10 ms. */
 #define SYSPROF_DEFAULT_INTERVAL 10
 #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin"
 
-- 
2.34.1

[Tarantool-patches] [PATCH luajit 3/4] sysprof: introduce specific errors and default mode

[Sergey Bronnikov via Tarantool-patches]

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.
---
 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"
 #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')
+        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)
 {
-  if (lua_gettop(L) != 1)
-    return PROFILE_ERRUSE;
+  if (lua_gettop(L) != 1) {
+    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")
 #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
+
 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")
+test:is(errno, nil, "errno with missed profiling mode")
+assert(misc.sysprof.stop(), "sysprof is not stopped")
+
+-- 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{
+    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

[Tarantool-patches] [PATCH luajit 4/4] sysprof: fix a message with stop without run

[Sergey Bronnikov via Tarantool-patches]

The function `misc.sysprof.stop()` reports that profiler is
already running:

| $ ./build/src/luajit -e 'print(misc.sysprof.stop())'
| nil     profiler is running already     22

The patch fixes that.
---
 src/lj_sysprof.c                                             | 2 +-
 test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
index 88c7a41b..b76f503c 100644
--- a/src/lj_sysprof.c
+++ b/src/lj_sysprof.c
@@ -493,7 +493,7 @@ int lj_sysprof_stop(lua_State *L)
   global_State *g = sp->g;
   struct lj_wbuf *out = &sp->out;
 
-  if (SPS_IDLE == sp->state)
+  if (SPS_PROFILE != sp->state)
     return PROFILE_ERRRUN;
   else if (G(L) != g)
     return PROFILE_ERRUSE;
diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index 68a4b72f..91f9ca5c 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(33)
+test:plan(34)
 
 jit.off()
 -- XXX: Run JIT tuning functions in a safe frame to avoid errors
@@ -98,7 +98,8 @@ assert(res, err)
 
 -- Not running.
 res, err, errno = misc.sysprof.stop()
-test:ok(res == nil and err, "res and error with not running")
+test:is(res, nil, "res with not running")
+test:ok(err:match("profiler is running already"), "error with not running")
 test:ok(type(errno) == "number", "errno with not running")
 
 -- Bad path.
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 1/4] test: add descriptions to sysprof testcases

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, with a bunch of nits below.

On 04.02.25, Sergey Bronnikov wrote:
> The patch add descriptions to sysprof testcases to make TAP report

Typo: s/add/adds/
Typo: s/TAP/the TAP/

> more usable.
> ---
>  .../profilers/misclib-sysprof-lapi.test.lua   | 36 ++++++++++---------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> index b2c38c49..581fb7fd 100644
> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> @@ -65,34 +65,35 @@ end
>  
>  -- Wrong profiling mode.
>  local res, err, errno = misc.sysprof.start{ mode = "A" }
> -test:ok(res == nil and err:match("profiler misuse"))
> -test:ok(type(errno) == "number")
> +test:ok(res == nil and err:match("profiler misuse"), "res with no parameters")

Minor: s/res/result status/
Here and below.
Feel free to ignore.

It is not the "no parameters", but rather the wrong profiler mode.

> +test:ok(type(errno) == "number", "errno with no parameters")

It is not the "no parameters", but rather the wrong profiler mode.

>  
>  -- Already running.

<snipped>

>  -- Not running.

<snipped>

>  -- Bad path.
>  res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
> -test:ok(res == nil and err:match("No such file or directory"))
> -test:ok(type(errno) == "number")
> +test:ok(res == nil and err:match("No such file or directory"), "res and error with bad path")

Nit: Line length is more than 80 symbols.

> +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"))
> -test:ok(type(errno) == "number")
> +test:ok(res == nil and err:match("profiler misuse"), "res and error with bad interval")

Nit: Line length is more than 80 symbols.

> +test:ok(type(errno) == "number", "errno with bad interval")
>  
>  -- DEFAULT MODE
>  
> @@ -102,20 +103,23 @@ end
>  
>  local report = misc.sysprof.report()
>  
> --- Check the profile is not empty
> -test:ok(report.samples > 0)
> +-- Check the profile is not empty.
> +test:ok(report.samples > 0, "number of samples is greater than 0")

I would also add the following:
"... than 0 for the default payload"

>  -- There is a Lua function with FNEW bytecode in it. Hence there
>  -- are only three possible sample types:
>  -- * LFUNC -- Lua payload is sampled.
>  -- * GC -- Lua GC machinery triggered in scope of FNEW bytecode
>  --   is sampled.
>  -- * INTERP -- VM is in a specific state when the sample is taken.
> -test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0)
> +test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0,
> +        "total number of LFUNC, GC and INTERP samples is greater than 0")

I would also add the following:
"... than 0 for the default payload"

>  -- There is no fast functions and C function in default payload.
> -test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0)
> +test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0,
> +        "total number of FFUNC and CFUNC samples is equal to 0")

I would also add the following:
"... to 0 for the default payload"

>  -- Check all JIT-related VM states are not sampled.
>  for _, vmstate in pairs({ 'TRACE', 'RECORD', 'OPT', 'ASM', 'EXIT' }) do
> -  test:ok(report.vmstate[vmstate] == 0)
> +  local msg = ("total number of VM state %s is equal to 0"):format(vmstate)

I would also add the following:
"... to 0 for the default payload"

Minor: I would avoid the tmp variable here, since it is used only once
anyway.

> +  test:ok(report.vmstate[vmstate] == 0, msg)
>  end
>  
>  -- With very big interval.
> @@ -124,7 +128,7 @@ if not pcall(generate_output, { mode = "D", interval = 1000 }) then
>  end
>  
>  report = misc.sysprof.report()
> -test:ok(report.samples == 0)
> +test:ok(report.samples == 0, "total number of samples is equal to 0")

I would also add the following:
"... to 0 for the too big sampling interval"

>  
>  -- LEAF MODE
>  check_mode("L", 100)
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/4] sysprof: fix typo in the comment

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, with a few nits below regarding the commit message.

On 04.02.25, Sergey Bronnikov wrote:
> The patch fixes inconsistency in comment and source code below
> the comment. The typo was added in the initial commit
> 2593a9f83072 ("misc: introduce Lua API for new sampling profiler").

Please use the full commit hash.

I know that this is enough too, but:
a) It would be consistent with other commit messages.
b) There is less probability of the collision between hashes.

Maybe it is worth to mention "Follows up tarantool/tarantool#781" (the
sysprof ticket) if you want (I am not sure that this is needed, since it
is only the comment).
> ---
>  src/lib_misc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index ca1d1c75..5b7a4b62 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 3/4] sysprof: introduce specific errors and default mode

[Sergey Kaplun via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH luajit 4/4] sysprof: fix a message with stop without run

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 04.02.25, Sergey Bronnikov wrote:
> The function `misc.sysprof.stop()` reports that profiler is
> already running:
> 
> | $ ./build/src/luajit -e 'print(misc.sysprof.stop())'

Minor: You may omit a ./build here, but this is a matter of taste, so
feel free to ignore.

> | nil     profiler is running already     22
> 
> The patch fixes that.

Unfortunatelly not:

| $ git --no-pager log --oneline -n1 --no-decorate && ./src/luajit -e 'print(misc.sysprof.stop())'
| b49c5fac sysprof: fix a message with stop without run
| nil     profiler is running already     22

See my comments below.

Please add the follow-up to the tarantool/tarantool#781.

> ---
>  src/lj_sysprof.c                                             | 2 +-
>  test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 5 +++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
> index 88c7a41b..b76f503c 100644
> --- a/src/lj_sysprof.c
> +++ b/src/lj_sysprof.c
> @@ -493,7 +493,7 @@ int lj_sysprof_stop(lua_State *L)
>    global_State *g = sp->g;
>    struct lj_wbuf *out = &sp->out;
>  
> -  if (SPS_IDLE == sp->state)
> +  if (SPS_PROFILE != sp->state)

This check is correct. It checks that the profiler has no error or is
not running (i.e., that it is stopped). With the new check, a case with
an error during profiling will be masked.

It looks like we should add this case in the `misc.sysprof.stop()` (it
uses `sysprof_error()` which handles start cases).

>      return PROFILE_ERRRUN;
>    else if (G(L) != g)
>      return PROFILE_ERRUSE;
> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> index 68a4b72f..91f9ca5c 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(33)
> +test:plan(34)
>  
>  jit.off()
>  -- XXX: Run JIT tuning functions in a safe frame to avoid errors
> @@ -98,7 +98,8 @@ assert(res, err)
>  
>  -- Not running.
>  res, err, errno = misc.sysprof.stop()
> -test:ok(res == nil and err, "res and error with not running")
> +test:is(res, nil, "res with not running")
> +test:ok(err:match("profiler is running already"), "error with not running")

I suppose there is bad copy-pasting here. It should be: 'profiler is not
running'. This is why the test passes.

>  test:ok(type(errno) == "number", "errno with not running")
>  
>  -- Bad path.
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/4] test: add descriptions to sysprof testcases

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4677 bytes --]

Hi, Sergey,

thanks for review!

On 10.02.2025 17:51, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, with a bunch of nits below.
>
> On 04.02.25, Sergey Bronnikov wrote:
>> The patch add descriptions to sysprof testcases to make TAP report
> Typo: s/add/adds/
> Typo: s/TAP/the TAP/

Fixed both typos.


>> more usable.
>> ---
>>   .../profilers/misclib-sysprof-lapi.test.lua   | 36 ++++++++++---------
>>   1 file changed, 20 insertions(+), 16 deletions(-)
>>
>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> index b2c38c49..581fb7fd 100644
>> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> @@ -65,34 +65,35 @@ end
>>   
>>   -- Wrong profiling mode.
>>   local res, err, errno = misc.sysprof.start{ mode = "A" }
>> -test:ok(res == nil and err:match("profiler misuse"))
>> -test:ok(type(errno) == "number")
>> +test:ok(res == nil and err:match("profiler misuse"), "res with no parameters")
> Minor: s/res/result status/
Fixed.
> Here and below.
> Feel free to ignore.
>
> It is not the "no parameters", but rather the wrong profiler mode.
Fixed.
>
>> +test:ok(type(errno) == "number", "errno with no parameters")
> It is not the "no parameters", but rather the wrong profiler mode.
>
>>   
>>   -- Already running.
> <snipped>
>
>>   -- Not running.
> <snipped>
>
>>   -- Bad path.
>>   res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
>> -test:ok(res == nil and err:match("No such file or directory"))
>> -test:ok(type(errno) == "number")
>> +test:ok(res == nil and err:match("No such file or directory"), "res and error with bad path")
> Nit: Line length is more than 80 symbols.
Fixed and sent a separate patch that limits max len by 80 symbols.
>
>> +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"))
>> -test:ok(type(errno) == "number")
>> +test:ok(res == nil and err:match("profiler misuse"), "res and error with bad interval")
> Nit: Line length is more than 80 symbols.
Fixed.
>
>> +test:ok(type(errno) == "number", "errno with bad interval")
>>   
>>   -- DEFAULT MODE
>>   
>> @@ -102,20 +103,23 @@ end
>>   
>>   local report = misc.sysprof.report()
>>   
>> --- Check the profile is not empty
>> -test:ok(report.samples > 0)
>> +-- Check the profile is not empty.
>> +test:ok(report.samples > 0, "number of samples is greater than 0")
> I would also add the following:
> "... than 0 for the default payload"
Fixed.
>
>>   -- There is a Lua function with FNEW bytecode in it. Hence there
>>   -- are only three possible sample types:
>>   -- * LFUNC -- Lua payload is sampled.
>>   -- * GC -- Lua GC machinery triggered in scope of FNEW bytecode
>>   --   is sampled.
>>   -- * INTERP -- VM is in a specific state when the sample is taken.
>> -test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0)
>> +test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0,
>> +        "total number of LFUNC, GC and INTERP samples is greater than 0")
> I would also add the following:
> "... than 0 for the default payload"
Fixed.
>>   -- There is no fast functions and C function in default payload.
>> -test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0)
>> +test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0,
>> +        "total number of FFUNC and CFUNC samples is equal to 0")
> I would also add the following:
> "... to 0 for the default payload"
Fixed.
>
>>   -- Check all JIT-related VM states are not sampled.
>>   for _, vmstate in pairs({ 'TRACE', 'RECORD', 'OPT', 'ASM', 'EXIT' }) do
>> -  test:ok(report.vmstate[vmstate] == 0)
>> +  local msg = ("total number of VM state %s is equal to 0"):format(vmstate)
> I would also add the following:
> "... to 0 for the default payload"
>
> Minor: I would avoid the tmp variable here, since it is used only once
> anyway.
Fixed and moved out of loop.
>> +  test:ok(report.vmstate[vmstate] == 0, msg)
>>   end
>>   
>>   -- With very big interval.
>> @@ -124,7 +128,7 @@ if not pcall(generate_output, { mode = "D", interval = 1000 }) then
>>   end
>>   
>>   report = misc.sysprof.report()
>> -test:ok(report.samples == 0)
>> +test:ok(report.samples == 0, "total number of samples is equal to 0")
> I would also add the following:
> "... to 0 for the too big sampling interval"

Fixed.


>>   
>>   -- LEAF MODE
>>   check_mode("L", 100)
>> -- 
>> 2.34.1
>>

[-- Attachment #2: Type: text/html, Size: 8162 bytes --]

Re: [Tarantool-patches] [PATCH luajit 2/4] sysprof: fix typo in the comment

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

Hi, Sergey!

thanks for review! Fixed and force-pushed.

On 10.02.2025 17:51, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, with a few nits below regarding the commit message.
>
> On 04.02.25, Sergey Bronnikov wrote:
>> The patch fixes inconsistency in comment and source code below
>> the comment. The typo was added in the initial commit
>> 2593a9f83072 ("misc: introduce Lua API for new sampling profiler").
> Please use the full commit hash.
Fixed.
>
> I know that this is enough too, but:
> a) It would be consistent with other commit messages.
> b) There is less probability of the collision between hashes.
>
> Maybe it is worth to mention "Follows up tarantool/tarantool#781" (the
> sysprof ticket) if you want (I am not sure that this is needed, since it
> is only the comment).
Added.
>> ---
>>   src/lib_misc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/lib_misc.c b/src/lib_misc.c
>> index ca1d1c75..5b7a4b62 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
> <snipped>
>
>> -- 
>> 2.34.1
>>

[-- Attachment #2: Type: text/html, Size: 2187 bytes --]

Re: [Tarantool-patches] [PATCH luajit 4/4] sysprof: fix a message with stop without run

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3004 bytes --]

Hi, Sergey!

thanks for valuable comments!

Sergey

On 10.02.2025 17:52, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On 04.02.25, Sergey Bronnikov wrote:
>> The function `misc.sysprof.stop()` reports that profiler is
>> already running:
>>
>> | $ ./build/src/luajit -e 'print(misc.sysprof.stop())'
> Minor: You may omit a ./build here, but this is a matter of taste, so
> feel free to ignore.
Removed 'build'.
>
>> | nil     profiler is running already     22
>>
>> The patch fixes that.
> Unfortunatelly not:
>
> | $ git --no-pager log --oneline -n1 --no-decorate && ./src/luajit -e 'print(misc.sysprof.stop())'
> | b49c5fac sysprof: fix a message with stop without run
> | nil     profiler is running already     22
>
> See my comments below.
>
> Please add the follow-up to the tarantool/tarantool#781.
Added.
>
>> ---
>>   src/lj_sysprof.c                                             | 2 +-
>>   test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 5 +++--
>>   2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
>> index 88c7a41b..b76f503c 100644
>> --- a/src/lj_sysprof.c
>> +++ b/src/lj_sysprof.c
>> @@ -493,7 +493,7 @@ int lj_sysprof_stop(lua_State *L)
>>     global_State *g = sp->g;
>>     struct lj_wbuf *out = &sp->out;
>>   
>> -  if (SPS_IDLE == sp->state)
>> +  if (SPS_PROFILE != sp->state)
> This check is correct. It checks that the profiler has no error or is
> not running (i.e., that it is stopped). With the new check, a case with
> an error during profiling will be masked.
>
> It looks like we should add this case in the `misc.sysprof.stop()` (it
> uses `sysprof_error()` which handles start cases).
Fixed.
>
>>       return PROFILE_ERRRUN;
>>     else if (G(L) != g)
>>       return PROFILE_ERRUSE;
>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> index 68a4b72f..91f9ca5c 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(33)
>> +test:plan(34)
>>   
>>   jit.off()
>>   -- XXX: Run JIT tuning functions in a safe frame to avoid errors
>> @@ -98,7 +98,8 @@ assert(res, err)
>>   
>>   -- Not running.
>>   res, err, errno = misc.sysprof.stop()
>> -test:ok(res == nil and err, "res and error with not running")
>> +test:is(res, nil, "res with not running")
>> +test:ok(err:match("profiler is running already"), "error with not running")
> I suppose there is bad copy-pasting here. It should be: 'profiler is not
> running'. This is why the test passes.
Fixed.
>
>>   test:ok(type(errno) == "number", "errno with not running")
>>   
>>   -- Bad path.
>> -- 
>> 2.34.1
>>

[-- Attachment #2: Type: text/html, Size: 4554 bytes --]

Re: [Tarantool-patches] [PATCH luajit 3/4] sysprof: introduce specific errors and default mode

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 14998 bytes --]

Hi, Sergey,

thanks for the comments!

On 10.02.2025 17:51, Sergey Kaplun via Tarantool-patches wrote:
> 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.

Also, sysprof has options that have no meaning for memprof:

mode and interval, and sysprof accepts a table when memprof accepts

a string. It is not considered as a problem for you, but specific messages

is a problem. Seems there is an inconsistency.


> Plus, the error message is a little
> bit incompatible with the previous version (you may notice this in test
> changes).

We discussed this verbally (you, Sergos and me) and decided that it is

not a problem.

> 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.

Honestly, it becomes more complicated in code, because error messages 
are not error messages,

but error prefixes, and now you should care about two things: error 
message and details.

> 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.
Anyway, I agree that introducing error details is a compromise in a 
current situtation.
> 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()`).
Merged.
> 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.
It is not reproduced with updated patchset.
>
>> ---
>>   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?
Fixed. (I remember you told me that would be good to set "C" as default 
mode)
>>   #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.


Fixed by checking a string len:

        }
        mode = strVdata(mode_opt);
-      if (mode[1] != '\0') {
+      if (strlen(mode) > 0 && mode[1] != '\0') {
          *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
          return PROFILE_ERRUSE;
        }
(1/1) Stage this h

>> +        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.
Removed parse_options.
>
>>   {
>> -  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.

Good advice! Thanks, fixed.

@@ -180,8 +180,11 @@ static int set_output_path(const char *path, struct 
luam_Sysprof_Options *opt) {

  static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
                                   const char **err_details) {
-  if (lua_gettop(L) != 1) {
-    lua_createtable(L, 0, 0);
+  int n = (int)(L->top - L->base);
+  if (n != 1) {
+    opt->mode = LUAM_SYSPROF_DEFAULT;
+    opt->interval = SYSPROF_DEFAULT_INTERVAL;
+    goto set_path;
    }

    if (!lua_istable(L, 1)) {
@@ -241,6 +244,8 @@ static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
      }
    }

+set_path:
+
    /* Get output path. */
    if (opt->mode != LUAM_SYSPROF_DEFAULT)
    {

>
> 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.
Fixed, thanks!
>
>> +    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.

Fixed.

>
>>   #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.
these additional err codes were removed
>
>> +
>>   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/
Fixed.
>
>> +test:is(errno, nil, "errno with missed profiling mode")
> Typo: s/errno/no errno/
Fixed.
>> +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.
Removed.
>
>> +
>> +-- 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.
Fixed.
>
>> +    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
>>

[-- Attachment #2: Type: text/html, Size: 19181 bytes --]