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 6950D6F3C7; Fri, 26 Mar 2021 13:10:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6950D6F3C7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1616753447; bh=FUaCC9DN0FxtGEHvC98vXUpAJCUXaXe2ICAC0tRIgzM=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=yiDQ1pLHCRprz9KWIRGapocovH7GqSJDdIQ60Fmtf64eBAr9xrIKsNWMUDBduF88r jey/LzKANwzCczTkNquKAu+KY0xL4E2zusSOy/F62greX0lWJkK6V40iq6iuWElGqb Qf6NWcfZPBTdZ8Z1sjBbZQm4x2kFvYzLe2jzky4o= Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D75326F3C7 for ; Fri, 26 Mar 2021 13:10:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D75326F3C7 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lPjQS-0003R0-Jo; Fri, 26 Mar 2021 13:10:45 +0300 Date: Fri, 26 Mar 2021 13:10:35 +0300 To: Sergey Kaplun Message-ID: <20210326101035.GH29703@tarantool.org> References: <20210309175422.25432-1-skaplun@tarantool.org> <20210325201459.GG29703@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9064ADF4728AA0EE993662DB34D9B1ABD5A9410DDDE83AB43182A05F538085040C659E06F13332CCA4DE3A798B2618EE71107B3A1898973DE0094FC54F02B2CD7 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7850A3F981F25E362EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063748E7A03516F25E8E8638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C5DD32608FC869F5DBCB32C3ABD0C640FF676E971DCEAB8EFA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C34964A708C60C975A117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947C0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E73528A6D463EDFD0DBBC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A50963C22CCCDFEC8AAB14D30BB5A3790BFFBDEE7E71D649ADD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D341E2D05735FCBECD1C7A436914D2392993BEA75FE61C72D9DCCCFEFB2A53DB1EE27EC0C602B063C651D7E09C32AA3244CD2840C4D0D696047BEA4149F3EDBAE00C3B3ADDA61883BB5927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojapPp7P/VpAiUuqVpyQYcmw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A06382235F8AE685F5CE70328FD542C6E62CF7DA7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] memprof: report stack resizing as internal event 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, On 26.03.21, Sergey Kaplun wrote: > Igor, > > Thanks for the review! > > On 25.03.21, Igor Munkin wrote: > > Sergey, > > > > Thanks for the patch! Please consider my comments below. > > > > See the new commit message below, branch is force-pushed: > > =================================================================== > memprof: report stack resizing as internal event > > Resizing of the Lua stack is not reported as internal allocation > as it should. As a result, it may lead to crash inside Lua or FF > frames. > > When the memory profiler runs, reallocation occurs first, and after > profiler reports the corresponding event. When the stack is resized for > local function arguments, the link to previous the frame is invalid in Typo: s/to previous the frame/to the previous frame/. > the case of reallocation. Therefore, the assertion in `debug_framepc()` > fails. > > Resolves tarantool/tarantool#5842 > Follows up tarantool/tarantool#5442 > =================================================================== > > > > > > > Resolves tarantool/tarantool#5842 > > > Follows up tarantool/tarantool#5442 > > > --- > > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5842-memprof-core-on-resizestack > > > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5842-memprof-core-on-resizestack > > > Issue: https://github.com/tarantool/tarantool/issues/5842 > > > > > > > > > src/lj_state.c | 6 ++++++ > > > .../misclib-memprof-lapi.test.lua | 18 ++++++++++++++++++ > > > 2 files changed, 24 insertions(+) > > > > > > diff --git a/src/lj_state.c b/src/lj_state.c > > > index 1ed79a5..ea9abd4 100644 > > > --- a/src/lj_state.c > > > +++ b/src/lj_state.c > > > @@ -64,7 +64,11 @@ static void resizestack(lua_State *L, MSize n) > > > MSize oldsize = L->stacksize; > > > MSize realsize = n + 1 + LJ_STACK_EXTRA; > > > GCobj *up; > > > + int32_t old_vmstate = G(L)->vmstate; > > > > Please consider the naming and the workflow in lj_gc.c for such > > situations: G(L) is stored into a separate variable and is > > named . It makes grep for such spots much easier, doesn't it? > > You can see more by grepping vmstate. Made naming more consistent I did it before sending the review. I did it again now. My opinion is not changed. This is "idiomatic" approach to "push" and "pop" vmstate used only in lj_gc.c since this is not required elsewhere. If you move that approach "intact", it shows that the semantics of your code are the same. Otherwise, every other occurrence of such vmstate "pushing" and "popping" allows to introduce own naming: pstate, prevstate, prev_vmstate, sstate, save_state, savevmstate -- there are lots of combinations. And nobody can stop contributor from this, since it is "more consistent for the current TU". "Feel free to prove the opposite"(c) :) The current naming is much better than the previous one, but I still propose to save the original on and save G(L) into a new variable. > with the . s/old_vmstate/oldvmstate/g > > See the iterative patch below. > > > > > > + > > > lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1); > > > + > > > + setvmstate(G(L), INTERP); > > > > We didn't notice this before. Now you leave not a single word regarding > > this hack. How come? > > Added the comment. But I don't get how it is connected to our notice. > May be it should be mentioned in docs? I meant that we pushed this bug into the trunk and didn't notice it. Comment is totally enough, thanks! > > See the iterative patch below. Branch is force-pushed. > =================================================================== > diff --git a/src/lj_state.c b/src/lj_state.c > index ea9abd4..c86e098 100644 > --- a/src/lj_state.c > +++ b/src/lj_state.c > @@ -64,10 +64,15 @@ static void resizestack(lua_State *L, MSize n) > MSize oldsize = L->stacksize; > MSize realsize = n + 1 + LJ_STACK_EXTRA; > GCobj *up; > - int32_t old_vmstate = G(L)->vmstate; > + int32_t oldvmstate = G(L)->vmstate; > > lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1); > > + /* > + ** Lua stack is inconsistent durent reallocation, profilers Typo: s/profilers/profiler/ or s/depends/depend/. > + ** depends on vmstate during reports, so set vmstate to INTERP > + ** to avoid inconsistent behaviour. > + */ > setvmstate(G(L), INTERP); > st = (TValue *)lj_mem_realloc(L, tvref(L->stack), > (MSize)(oldsize*sizeof(TValue)), > @@ -85,7 +90,7 @@ static void resizestack(lua_State *L, MSize n) > for (up = gcref(L->openupval); up != NULL; up = gcnext(up)) > setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta)); > > - G(L)->vmstate = old_vmstate; > + G(L)->vmstate = oldvmstate; > } > > /* Relimit stack after error, in case the limit was overdrawn. */ > =================================================================== > > > > > > st = (TValue *)lj_mem_realloc(L, tvref(L->stack), > > > (MSize)(oldsize*sizeof(TValue)), > > > (MSize)(realsize*sizeof(TValue))); > > > @@ -80,6 +84,8 @@ static void resizestack(lua_State *L, MSize n) > > > L->top = (TValue *)((char *)L->top + delta); > > > for (up = gcref(L->openupval); up != NULL; up = gcnext(up)) > > > setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta)); > > > + > > > + G(L)->vmstate = old_vmstate; > > > } > > > > > > /* Relimit stack after error, in case the limit was overdrawn. */ > > > -- > > > 2.28.0 > > > > > > > -- > > Best regards, > > IM > > -- > Best regards, > Sergey Kaplun -- Best regards, IM