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

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

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.

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

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

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


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