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

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

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.

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

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

Also, `is()` check is more verbose in case of failure -- it prints got
and expected values.

> >
> >
> >> +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
> > <snipped>
> >
> >> 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
> > <snipped>
> >
> >> 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
> > <snipped>
> >
> >> -- 
> >> 2.43.0
> >>

-- 
Best regards,
Sergey Kaplun