Hi, Sergey, See my answers below. Sergey On 3/12/26 20:59, Sergey Kaplun wrote: > Hi, Sergey! > See my answers below. > > On 12.03.26, Sergey Bronnikov wrote: >> Hi, Sergey, >> >> thanks for review! See my comments. >> >> Sergey >> >> On 3/12/26 15:00, Sergey Kaplun via Tarantool-patches wrote: >>> Hi, Sergey! >>> Thanks for the patch! >>> Please consider my comments below. >>> >>> On 22.01.26, Sergey Bronnikov wrote: >>>> The patch introduce flags in module "misc" with support status >>>> for sysprof and memprof: `misc.sysprof.enabled` and >>>> `misc.memprof.enabled`. Both flags are boolean and always >>> Let's rename it to `available` instead. The `enabled` may be interpreted >>> as `is_running`, and confuse the user then. >> I propose using `is_available` instead. > The Lua API has 0 functions in the snake_case. Even `funcinfo`, > `traceexitstub` from lib_jit.c have this naming style. Lets be > consistent with it, especially, since `is_` prefix doesn't change the > semantics. ok, let's rename to "available". diff --git a/src/lib_misc.c b/src/lib_misc.c index 6b2278c1..3463445c 100644 --- a/src/lib_misc.c +++ b/src/lib_misc.c @@ -479,10 +479,10 @@ LUALIB_API int luaopen_misc(struct lua_State *L)  #if !LJ_TARGET_WINDOWS    LJ_LIB_REG(L, LUAM_MISCLIBNAME ".memprof", misc_memprof);    lua_pushboolean(L, LJ_HASMEMPROF); -  lua_setfield(L, -2, "enabled"); +  lua_setfield(L, -2, "available");    LJ_LIB_REG(L, LUAM_MISCLIBNAME ".sysprof", misc_sysprof);    lua_pushboolean(L, LJ_HASSYSPROF); -  lua_setfield(L, -2, "enabled"); +  lua_setfield(L, -2, "available");  #endif /* !LJ_TARGET_WINDOWS */    return 1;  } diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua index f867cfc6..2d2d43c1 100644 --- a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua @@ -19,7 +19,8 @@ test:ok(err:match('profiler is disabled'),          'error on stop when memprof is disabled') test:ok(type(errno) == 'number', 'errno on start when memprof is disabled') -test:ok(type(misc.memprof.enabled) == 'boolean', 'misc.memprof.enabled exists') -test:ok(misc.memprof.enabled == false, 'misc.memprof.enabled is false') +test:ok(type(misc.memprof.available) == 'boolean', +        'misc.memprof.available exists') +test:ok(misc.memprof.available == false, 'misc.memprof.available is false') test:done(true) diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua index 44ba8b08..a7aad80e 100644 --- a/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua @@ -270,7 +270,8 @@ test:test("jit-output", function(subtest)    jit.opt.start(unpack(jit_opt_default))  end) -test:ok(type(misc.memprof.enabled) == 'boolean', 'misc.memprof.enabled exists') -test:ok(misc.memprof.enabled == true, 'misc.memprof.enabled is true') +test:ok(type(misc.memprof.available) == 'boolean', +        'misc.memprof.available exists') +test:ok(misc.memprof.available == true, 'misc.memprof.available is true') test:done(true) diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua index c023d8f1..5b9cc269 100644 --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua @@ -26,7 +26,8 @@ test:ok(err:match('profiler is disabled'),          'error on stop when sysprof is disabled') test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled') -test:ok(type(misc.sysprof.enabled) == 'boolean', 'misc.sysprof.enabled exists') -test:ok(misc.sysprof.enabled == false, 'misc.sysprof.enabled is false') +test:ok(type(misc.sysprof.available) == 'boolean', +        'misc.sysprof.available exists') +test:ok(misc.sysprof.available == false, 'misc.sysprof.available is false') test:done(true) diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua index 41ed43e0..86115b62 100644 --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua @@ -228,7 +228,8 @@ check_mode("C", 100)  os.remove(TMP_BINFILE) -test:ok(type(misc.sysprof.enabled) == 'boolean', 'misc.sysprof.enabled exists') -test:ok(misc.sysprof.enabled == true, 'misc.sysprof.enabled is true') +test:ok(type(misc.sysprof.available) == 'boolean', +        'misc.sysprof.available exists') +test:ok(misc.sysprof.available == true, 'misc.sysprof.available is true') test:done(true) >>>> available on platforms supported by profilers (Windows is not >>>> supported). >>>> >>>> Closes tarantool/tarantool#12215 >>> Minor: Should be Resolves, since it is closed when we bump LuaJIT in the >>> Tarantool repository. >> Ok, updated. >>>> --- >>>> src/lib_misc.c | 4 ++++ >>>> .../profilers/misclib-memprof-lapi-disabled.test.lua | 5 ++++- >>>> test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua | 5 ++++- >>>> .../profilers/misclib-sysprof-lapi-disabled.test.lua | 5 ++++- >>>> test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 5 ++++- >>>> 5 files changed, 20 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/src/lib_misc.c b/src/lib_misc.c >>>> index 034ff878..6b2278c1 100644 >>>> --- a/src/lib_misc.c >>>> +++ b/src/lib_misc.c >>>> @@ -478,7 +478,11 @@ LUALIB_API int luaopen_misc(struct lua_State *L) >>>> LJ_LIB_REG(L, LUAM_MISCLIBNAME, misc); >>>> #if !LJ_TARGET_WINDOWS >>>> LJ_LIB_REG(L, LUAM_MISCLIBNAME ".memprof", misc_memprof); >>>> + lua_pushboolean(L, LJ_HASMEMPROF); >>>> + lua_setfield(L, -2, "enabled"); >>>> LJ_LIB_REG(L, LUAM_MISCLIBNAME ".sysprof", misc_sysprof); >>>> + lua_pushboolean(L, LJ_HASSYSPROF); >>>> + lua_setfield(L, -2, "enabled"); >>> Is it possible to use standard `LJLIB_PUSH() LJLIB_SET()` machinery >>> instead? >> I didn't get what is a macros `LJLIB_SET`. >> >> Also, what are the benefits with using mentioned macros instead more >> standard Lua API functions? > It is more consistent with the rest of the code base. > Also, it helps to avoid table rehasing on library initialization. > You may see details in buildvm_lib.c > Updated: diff --git a/src/lib_misc.c b/src/lib_misc.c index 3463445c..62886242 100644 --- a/src/lib_misc.c +++ b/src/lib_misc.c @@ -389,6 +389,8 @@ LJLIB_CF(misc_sysprof_report)  #endif /* !LJ_HASSYSPROF */  } +LJLIB_PUSH(top-2) LJLIB_SET(available) +  /* ----- misc.memprof module ---------------------------------------------- */  #define LJLIB_MODULE_misc_memprof @@ -459,6 +461,8 @@ LJLIB_CF(misc_memprof_stop)  }  #endif /* !LJ_TARGET_WINDOWS */ +LJLIB_PUSH(top-2) LJLIB_SET(available) +  #include "lj_libdef.h"  /* ------------------------------------------------------------------------ */ @@ -477,12 +481,10 @@ LUALIB_API int luaopen_misc(struct lua_State *L)    LJ_LIB_REG(L, LUAM_MISCLIBNAME, misc);  #if !LJ_TARGET_WINDOWS -  LJ_LIB_REG(L, LUAM_MISCLIBNAME ".memprof", misc_memprof);    lua_pushboolean(L, LJ_HASMEMPROF); -  lua_setfield(L, -2, "available"); -  LJ_LIB_REG(L, LUAM_MISCLIBNAME ".sysprof", misc_sysprof); +  LJ_LIB_REG(L, LUAM_MISCLIBNAME ".memprof", misc_memprof);    lua_pushboolean(L, LJ_HASSYSPROF); -  lua_setfield(L, -2, "available"); +  LJ_LIB_REG(L, LUAM_MISCLIBNAME ".sysprof", misc_sysprof);  #endif /* !LJ_TARGET_WINDOWS */    return 1;  } >> >>>> #endif /* !LJ_TARGET_WINDOWS */ >>>> return 1; >>>> } >>>> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua >>>> index de0aa136..f867cfc6 100644 >>>> --- a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua >>>> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua >>>> @@ -3,7 +3,7 @@ local test = tap.test('misclib-memprof-lapi-disabled'):skipcond({ >>>> ['Memprof is enabled'] = not os.getenv('LUAJIT_DISABLE_MEMPROF'), >>>> }) >>>> >>>> -test:plan(6) >>>> +test:plan(8) >>>> >>>> -- Attempt to start memprof when it is disabled. >>>> local res, err, errno = misc.memprof.start() >>>> @@ -19,4 +19,7 @@test:ok(err:match('profiler is disabled'), >>>> 'error on stop when memprof is disabled') >>>> test:ok(type(errno) == 'number', 'errno on start when memprof is disabled') >>>> >>>> +test:ok(type(misc.memprof.enabled) == 'boolean', 'misc.memprof.enabled exists') >>> I suppose that >>> |test:is(misc.memprof.available, false, 'misc.memprof.enabled correct') >>> is enough. >>> >>> Same for other tests below. >> if "misc.memprof" is not a table the error will be "attempt to index field" > Don't get the point here: > 1) We will see this error much earlier in that case. > 2) We will see with your approach as well. > > I don't get the reason for the `type()` call if the check below will > fail with a different type as well (since Lua checks types first). I see > no reason in double-checking that means nothing. The same sense as with checking both boolean value for pcall and an error message, see for example your patch in [1]: +test:ok(not result, 'correct status for recursive call') +test:like(errmsg, 'stack overflow', 'correct error message for recursive call') In aforementioned patch you can check only message, but you check both values. 1. https://lists.tarantool.org/tarantool-patches/20260316104853.23901-1-skaplun@tarantool.org/T/#u > > Also, `is()` check is more verbose in case of failure -- it prints got > and expected values. It's much easier for me to read that a type check failed than to figure out why the value is incorrect. The TAP checks produce completely unreadable message; I never use them. >>> >>>> +test:ok(misc.memprof.enabled == false, 'misc.memprof.enabled is false') >>>> + >>>> test:done(true) >>>> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua >>>> index cd675864..44ba8b08 100644 >>>> --- a/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua >>>> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua >>> >>> >>>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua >>>> index 2a9ce796..c023d8f1 100644 >>>> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua >>>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua >>> >>> >>>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >>>> index 701d58e4..41ed43e0 100644 >>>> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >>>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >>> >>> >>>> -- >>>> 2.43.0 >>>>