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

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


>
>>   #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"
>
>
>> +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: 6777 bytes --]