Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

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

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.

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/#u

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

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>

-- 
Best regards,
Sergey Kaplun