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 17A7A6F3C7; Fri, 26 Mar 2021 17:10:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 17A7A6F3C7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1616767841; bh=FocKSZoRaoH+vqdBZSPX0qCZdvc79beNjiuwfZ5EfL8=; 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=uotjKOCOuldMS+SX8Zc+sha/vnklZoGn5NGCr/YJ9WLXvFniH/pE3vADYIe0Fg/fx C2fFYKUgkQWE+QlECBSGGXym4K0V7ToNMh6sjoXRauBg9ej5wDpxA8/JvYUBcEKO7b T2IGQoET0I6YAU09CX00I8rr8x28mfEy/2MXu4G0= Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 152AA6F3C7 for ; Fri, 26 Mar 2021 17:10:40 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 152AA6F3C7 Received: by smtp29.i.mail.ru with esmtpa (envelope-from ) id 1lPnAd-0007tr-0C; Fri, 26 Mar 2021 17:10:39 +0300 Date: Fri, 26 Mar 2021 17:09:46 +0300 To: Igor Munkin Message-ID: References: <20210309175422.25432-1-skaplun@tarantool.org> <20210325201459.GG29703@tarantool.org> <20210326101035.GH29703@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210326101035.GH29703@tarantool.org> X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9064ADF4728AA0EE9AECA9F3C9C9885BEE78E91CF33279E24182A05F53808504049A338D30BA6B43BE2D370C034A79584B229E7343D9E87BB37CFCE2AAE0C5764 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7EA4B66823129EB3CEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637BC9F889FE9D346E88638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C5DD32608FC869F5DBC533FA6ACCC1EEB5986B60DB817CCF8A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3E478A468B35FE767117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947C0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E73528A6D463EDFD0DBBC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5555CA03615ED1A1C1790A3942B799CDC4BD52D6CB43283BAD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3455AC8BF8E3153BA0DE3B6948C09766AB3C0096B54D3D149C1A7C705A82360C5083A4E3850A3437CB1D7E09C32AA3244C1920A8A2FD031318AB2D37B4C799FC9F95A9E0DC41E9A4CFFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojapPp7P/VpAimYy1aAa1tQQ== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB4FD8785D29FECD3AAF018023456F2B95B1C93206F0F26F3DEF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Igor, I've fixed typos in the commit message and the comment. On 26.03.21, Igor Munkin wrote: > 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/. Fixed. > > > the case of reallocation. Therefore, the assertion in `debug_framepc()` > > fails. > > > > Resolves tarantool/tarantool#5842 > > Follows up tarantool/tarantool#5442 > > =================================================================== The new commit message is the following: =================================================================== 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 the previous frame is invalid in 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 But not only in some old value is saved to be reused later: | $ grep -rn -P '\Wold[A-Za-z]' src/l*.c | grep -v -e define -e "\*\*" -e "\*/" -e "\*/" | wc -l | 123 Instead one-letter abbreviation (grep out comments and most frequently used constructions): | $ grep -rn -P '\Wo[^l]' src/l*.c | grep -v -e "\Wo\W" -e "\Wo[12]" | grep -v \ | -e obj -e open -e op -e opt -e octet -e out -e on \ | -e ofs -e offs -e ok -e oddspill -e os -e orign \ | -e define -e overhead -e or \ | -e "\*/" -e "\*\*" -e "/\*" | wc -l | 25 The huge part of them was introduced via memory profiler by us. So, looks like your codestyle suggestion contradicts to codestyle of LuaJIT as it is. It is not described anywhere else, so looks like that source of true is the sources by themseles. > that approach "intact", it shows that the semantics of your code are the It's already the same, it is obvious by reading, plus comment describes the behaviour. > 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 None of them is usual in use for LuaJIT's C codebase, so they can't be used. > combinations. And nobody can stop contributor from this, since it is > "more consistent for the current TU". But with your approach naming will be choosed not like the most usual, but "same in the other file". I think that it will lead to the code inconsistency even in one 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. Ignoring for now. > > > 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/. Fixed, force-pushed. =================================================================== diff --git a/src/lj_state.c b/src/lj_state.c index c86e098..5701572 100644 --- a/src/lj_state.c +++ b/src/lj_state.c @@ -70,7 +70,7 @@ static void resizestack(lua_State *L, MSize n) /* ** Lua stack is inconsistent durent reallocation, profilers - ** depends on vmstate during reports, so set vmstate to INTERP + ** depend on vmstate during reports, so set vmstate to INTERP ** to avoid inconsistent behaviour. */ setvmstate(G(L), INTERP); =================================================================== > > > + ** 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 -- Best regards, Sergey Kaplun