From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 115FA1A34FD2; Wed, 18 Mar 2026 15:03:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 115FA1A34FD2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1773835389; bh=tDsw7IWePAZ5m24VhxMTZfwbj9dpSIul2ymKpsaD250=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=tlqYQmix3qZLPWVZddueQd4uhggGvIUR8hjgVWarQh2r6ByfZHRMzVYEBnyVU5+8f dGBhaohsvZSqh+UxsMRilKZNLvMSmo+asJvOMGg+oVKS0Arm4dOAyYP/zF+7cBirku eKEHMrAN0XvxmQD+aN5w7I+hmXIL02X/GPyez9wY= Received: from send83.i.mail.ru (send83.i.mail.ru [89.221.237.178]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A53C16D4671 for ; Wed, 18 Mar 2026 15:03:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A53C16D4671 Received: by exim-smtp-57656fb979-m8zxr with esmtpa (envelope-from ) id 1w2pcA-000000008SR-1jtC; Wed, 18 Mar 2026 15:03:06 +0300 Date: Wed, 18 Mar 2026 15:04:04 +0300 To: Sergey Bronnikov Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Message-ID: References: <7f14d93b-f295-434f-898c-3409daee3053@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9A2BFDE4DD2630CDAD330564038340474D4475F1EBC73B611182A05F5380850404D1E7F98FB1E8EC63DE06ABAFEAF6705674F33DC9C0BEAE60B6F3165720339AE59F445DFCDA6A35E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AC4684DF4EC4B256EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB55337566BCD8AD057D84402C009783DD83C12D2281DE40D3CC28C28DD93FF56A8526CCEC389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0A3E989B1926288338941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B652D31B9D28593E51CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249EDF998CB16CCEE6976E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8B7FE970921847EE7F3AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E73557739F23D657EF2BB5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5663DB33389F496335002B1117B3ED696675287FC19944B5630C8F815570A3530823CB91A9FED034534781492E4B8EEAD0AA277257C6A5E3DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0AD73CAD6646DEDE1918E10F71CB4DF9F96AB70F9BE574AE9C625B6776AC983F447FC0B9F89525902EE6F57B2FD27647F25E66C117BDB76D659334B61B263239D5FAD753283C4E7313FA807332E12F236DCB79D9E09A37AC20B4ED70B0FBCEDE702B8341EE9D5BE9A0AE935F2C705F9630BDC786D11CB86CF69138308CB0F6714A86536EB022892E5344C41F94D744909CECFA6C6B0C050A61A8CAF69B82BA93681CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVdbVVJCphTR/CWD2hffDeFc= X-Mailru-Sender: 520A125C2F17F0B17094CDC02B85F11BFDF717641029B0163DE06ABAFEAF6705674F33DC9C0BEAE6B7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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. > >>>> --- > >>>> 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 > >>>> > >>>> +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') > >>>> + -- Best regards, Sergey Kaplun