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: 8666 bytes --]

Hi, Sergey,

See my comments below.

Sergey

On 3/18/26 15:04, Sergey Kaplun wrote:
> 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.

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

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

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