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 B64356F3C7; Fri, 26 Mar 2021 11:54:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B64356F3C7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1616748899; bh=ZJP/b+ocZPrULUs2HirCvdrgEXuHA3Ir63ONW/XfUpk=; 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=jTyPL+NmRztbSy+oMBpkENM1cBWv39XxWFAugk+WaV21AEuUMS7TgydnGlnuTnLp+ PLIjBZ1gn3krh1w1w/J1q8K1+v15WoyjamVfbPc1MdSfUV3QCKU1HU04XRAqfqqJvL mrtqB4MkqTfFeYYL2BtCBaWQ0NZveLuNSKLzW3TY= Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 A44636F3C7 for ; Fri, 26 Mar 2021 11:54:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A44636F3C7 Received: by smtp49.i.mail.ru with esmtpa (envelope-from ) id 1lPiF7-0003zo-Rp; Fri, 26 Mar 2021 11:54:58 +0300 Date: Fri, 26 Mar 2021 11:54:05 +0300 To: Igor Munkin Message-ID: References: <20210309175422.25432-1-skaplun@tarantool.org> <20210325201459.GG29703@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210325201459.GG29703@tarantool.org> X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9064ADF4728AA0EE9AECA9F3C9C9885BEE78E91CF33279E24182A05F5380850405BF73E82CF35D64F1D78E3A33D4165DC09A6D803666447EB87E04EB0EE68F107 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE74323F140F3EE5B6AC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7F68DA2F3749BB650EA1F7E6F0F101C67CDEEF6D7F21E0D1D174C73DBBBFC7664B4E7100369CBBC0712793EE1A3C0CC439AD9BF2882A88BAC389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6957A4DEDD2346B42CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB86D1867E19FE14079C09775C1D3CA48CFC5EA940A35A165FF2DBA43225CD8A89F890A246B268E114EC6EABA9B74D0DA47B5C8C57E37DE458BEDA766A37F9254B7 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C9783161489AFE8A708FD1654CE1D82DC659E2 X-C1DE0DAB: 0D63561A33F958A51A9C3C3EC64AF843B4131BABA55D141DE0277EBCF211D665D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3498EF79680EE3725C7D46344D9ECE67D3C28AD375F452686243747E3761AFA7D0D2DDFE3A96AC74C21D7E09C32AA3244CB20358C569D4697EC3BBBEE57CA5CFD48894E9C85370243EFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojapPp7P/VpAgc2LOrx4wgTw== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB4E39B6096977B92AA97E4A483EFE21B704EE14B57AC70EBE9F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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, Thanks for the review! On 25.03.21, Igor Munkin wrote: > Sergey, > > Thanks for the patch! Please consider my comments below. > > On 09.03.21, Sergey Kaplun wrote: > > Resizing of the Lua stack is not reported as internal allocation. > > Was it done intentionally? You claim this as a plain fact with no > rationale. If this is a bug (by design) then mention it explicitly here. It is obviously a bug. > Otherwise, you need to provide a reason why you change the semantics of > Lua stack reallocations in terms of memprof. For me it looks like a bug > we missed in the previous release: user has nothing to do with guest > stack reallocations (like with JIT related ones), hence these memory > manipulations are totally internal. Maybe you have another vision > regarding this. > > > Moreover, it may lead to crash inside Lua or FF frames. > > This is a symptom, not another reason. So "As a result" fits better than > "Moreover" here. Reworded. > > > > > Profiler performs reallocation first and after reports corresponding > > Profiler makes no allocations. I believe you would like to say that > reallocation occurs at first and after memprof reports the event. > > Typo: s/reports corresponding/reports the corresponding/. Fixed. > > > event. When the stack is resized for local function arguments, the link > > to previous frame is invalid in the cause of reallocation. Therefore, > > assertion in `debug_framepc()` failes, because of invalid function > > reference at previous frame. > > Typo: s/at previous/at the previous/. Fixed. > 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 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 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? 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 + ** 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. */ > > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua > > index 1c36c8a..93cc348 100644 > > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua > > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua > > @@ -125,5 +125,23 @@ test:ok(check_alloc_report(alloc, 25, 18, 100)) > > -- Collect all previous allocated objects. > > test:ok(free.INTERNAL.num == 102) > > > > +-- Test for https://github.com/tarantool/tarantool/issues/5842. > > +-- We do not interested in report itself. > > +misc.memprof.start("/dev/null") > > +-- We need to cause stack resize for local variables at function > > +-- call. Let's create a new coroutine (all slots are free). > > +-- It has 1 slot for dummy frame + 39 free slots + 5 extra slots > > +-- (so-called red zone) + 2 * LJ_FR2 slots. So 50 local variables > > +-- is enough. > > +local payload_str = "" > > +for i = 1, 50 do > > + payload_str = payload_str..("local v%d = %d\n"):format(i, i) > > +end > > +local f, errmsg = loadstring(payload_str) > > +assert(f, errmsg) > > Minor: you can use a TAP check here, but feel free to leave a plain > assert, since you expect works fine always. We test not `loadstring()` here, assert is for sure that there is no syntax error in the generated chunk. Ignoring. > > > +local co = coroutine.create(f) > > +coroutine.resume(co) > > +misc.memprof.stop() > > + > > jit.on() > > os.exit(test:check() and 0 or 1) > > -- > > 2.28.0 > > > > -- > Best regards, > IM -- Best regards, Sergey Kaplun