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

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

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

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

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