Hi, Sergey,
See my comments below.
Sergey
Sergey, Thanks for the fixes! LGTM, after fixing the last comment below. On 18.03.26, Sergey Bronnikov wrote: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 alwaysLet'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.<snipped>--- 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.cUpdated: 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) +Please, add here the: | #include "lj_libdef.h" How it is done for the others built-in. It is not crucial for now, but it helps to be consistent with other modules.
Updated:
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -391,6 +391,8 @@ LJLIB_CF(misc_sysprof_report)
LJLIB_PUSH(top-2) LJLIB_SET(available)
+#include "lj_libdef.h"
+
/* ----- misc.memprof module
---------------------------------------------- */
#define LJLIB_MODULE_misc_memprof
It is necessary to declare one module per time. Otherwise, the sysprof is declared together with the memprof module. That's not crucial, but let's be consistent along the codebase to avoid surprises in the future./* ----- 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<snipped>+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/#uThis is not quite the same, IMO. These checks test 2 __different__ returned values. 1 -- the status, 2 -- the error message. It is possible that the status is true, and there is no error message as well: | luajit -e 'print(pcall(error)); print(pcall(tonumber, "abc"))' | false nil | true nil So, if we get nil, we can distinguish these 2 cases. But from your point of view, the checks should look like the following: | test:ok(type(result) == 'boolean', 'correct status type') | test:ok(not result, 'correct status for recursive call') | test:ok(type(errmsg) == 'string', 'correct errmsg type') | test:like(errmsg, 'stack overflow', 'correct error message for recursive call') Note that in that case, if the type is incorrect, you should modify the test anyway to debug the behaviour (or inspect the test in the GDB). Side note: Anticipating your question: we don't check the type of the first returned value for the `pcall()` because it isn't the unit test for `pcall()`. Hence, we expect that it follows the behaviour declared in the Lua RefMan.
It's possible that the value's type can change, and it won't be a Boolean value,
especially since we use a LJLIB_ machinery (imagine you change the "top-2"
to the "top-3", for example). I want the test to catch this as early as possible.
It's fair to say that in tests, I have an extreme lack of trust to the system under test, especially when a SUT is a LuaJIT.
It's easier to parse the results of a failed test there. These checks are cheap
and only take one line, so I don't understand why we spend so much time discussing this.
I don't insist on these changes but still don't understand your point, though.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.I use and it may be helpful. I don't insisth, though.+test:ok(misc.memprof.enabled == false, 'misc.memprof.enabled is false') +<snipped>