* [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status @ 2026-01-22 11:24 Sergey Bronnikov via Tarantool-patches 2026-01-22 13:49 ` Sergey Bronnikov via Tarantool-patches 2026-03-12 12:00 ` Sergey Kaplun via Tarantool-patches 0 siblings, 2 replies; 9+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2026-01-22 11:24 UTC (permalink / raw) To: tarantool-patches, Sergey Kaplun 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 available on platforms supported by profilers (Windows is not supported). Closes tarantool/tarantool#12215 --- 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"); #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') +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 @@ -11,7 +11,7 @@ local test = tap.test("misclib-memprof-lapi"):skipcond({ ["Memprof is disabled"] = os.getenv("LUAJIT_DISABLE_MEMPROF"), }) -test:plan(5) +test:plan(7) local jit_opt_default = { 3, -- level @@ -270,4 +270,7 @@ 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: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 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 @@ -3,7 +3,7 @@ local test = tap.test('misclib-sysprof-lapi-disabled'):skipcond({ ['Sysprof is enabled'] = not os.getenv('LUAJIT_DISABLE_SYSPROF'), }) -test:plan(9) +test:plan(11) -- Attempt to start sysprof when sysprof is disabled. local res, err, errno = misc.sysprof.start() @@ -26,4 +26,7 @@ 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: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 701d58e4..41ed43e0 100644 --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua @@ -10,7 +10,7 @@ local test = tap.test("misclib-sysprof-lapi"):skipcond({ ["Disabled due to #10803"] = os.getenv("LUAJIT_TEST_USE_VALGRIND"), }) -test:plan(45) +test:plan(47) jit.off() -- XXX: Run JIT tuning functions in a safe frame to avoid errors @@ -228,4 +228,7 @@ 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:done(true) -- 2.43.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status 2026-01-22 11:24 [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status Sergey Bronnikov via Tarantool-patches @ 2026-01-22 13:49 ` Sergey Bronnikov via Tarantool-patches 2026-03-12 12:00 ` Sergey Kaplun via Tarantool-patches 1 sibling, 0 replies; 9+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2026-01-22 13:49 UTC (permalink / raw) To: Sergey Bronnikov, tarantool-patches, Sergey Kaplun [-- Attachment #1: Type: text/plain, Size: 509 bytes --] On 1/22/26 14:24, Sergey Bronnikov via Tarantool-patches 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 > available on platforms supported by profilers (Windows is not > supported). > > Closes tarantool/tarantool#12215 > --- > Issue: https://github.com/tarantool/tarantool/issues/12215 Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-12215-profilers-flags [-- Attachment #2: Type: text/html, Size: 1192 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status 2026-01-22 11:24 [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status Sergey Bronnikov via Tarantool-patches 2026-01-22 13:49 ` Sergey Bronnikov via Tarantool-patches @ 2026-03-12 12:00 ` Sergey Kaplun via Tarantool-patches 2026-03-12 17:04 ` Sergey Bronnikov via Tarantool-patches 1 sibling, 1 reply; 9+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2026-03-12 12:00 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status 2026-03-12 12:00 ` Sergey Kaplun via Tarantool-patches @ 2026-03-12 17:04 ` Sergey Bronnikov via Tarantool-patches 2026-03-12 17:59 ` Sergey Kaplun via Tarantool-patches 0 siblings, 1 reply; 9+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2026-03-12 17:04 UTC (permalink / raw) To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status 2026-03-12 17:04 ` Sergey Bronnikov via Tarantool-patches @ 2026-03-12 17:59 ` Sergey Kaplun via Tarantool-patches 2026-03-18 10:47 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 1 reply; 9+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2026-03-12 17:59 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status 2026-03-12 17:59 ` Sergey Kaplun via Tarantool-patches @ 2026-03-18 10:47 ` Sergey Bronnikov via Tarantool-patches 2026-03-18 12:04 ` Sergey Kaplun via Tarantool-patches 0 siblings, 1 reply; 9+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2026-03-18 10:47 UTC (permalink / raw) To: Sergey Kaplun; +Cc: Sergey Bronnikov, tarantool-patches [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status 2026-03-18 10:47 ` Sergey Bronnikov via Tarantool-patches @ 2026-03-18 12:04 ` Sergey Kaplun via Tarantool-patches 2026-03-18 12:34 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 1 reply; 9+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2026-03-18 12:04 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches 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. 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. 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> -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status 2026-03-18 12:04 ` Sergey Kaplun via Tarantool-patches @ 2026-03-18 12:34 ` Sergey Bronnikov via Tarantool-patches 2026-03-18 14:42 ` Sergey Kaplun via Tarantool-patches 0 siblings, 1 reply; 9+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2026-03-18 12:34 UTC (permalink / raw) To: Sergey Kaplun; +Cc: Sergey Bronnikov, tarantool-patches [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status 2026-03-18 12:34 ` Sergey Bronnikov via Tarantool-patches @ 2026-03-18 14:42 ` Sergey Kaplun via Tarantool-patches 0 siblings, 0 replies; 9+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2026-03-18 14:42 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches Sergey, Thanks for the fix! LGTM. <snipped> > >>> 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 just want to understand your point of view. As I said before -- I don't insist on the changes. -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-03-18 14:41 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-01-22 11:24 [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status Sergey Bronnikov via Tarantool-patches 2026-01-22 13:49 ` Sergey Bronnikov via Tarantool-patches 2026-03-12 12:00 ` Sergey Kaplun via Tarantool-patches 2026-03-12 17:04 ` Sergey Bronnikov via Tarantool-patches 2026-03-12 17:59 ` Sergey Kaplun via Tarantool-patches 2026-03-18 10:47 ` Sergey Bronnikov via Tarantool-patches 2026-03-18 12:04 ` Sergey Kaplun via Tarantool-patches 2026-03-18 12:34 ` Sergey Bronnikov via Tarantool-patches 2026-03-18 14:42 ` Sergey Kaplun via Tarantool-patches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox