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 81BAB6EC59; Wed, 10 Mar 2021 20:38:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 81BAB6EC59 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1615397931; bh=TISfr5VW7RUl+lx1v/UDglK3zCs+8UtKDd+ki0gaOHM=; 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=bZwEociGgIvWI5BFdA8VN2hiuk7dq76jG3S2JN72OMA8ulY6hAdTYoGBuT1IZiPHp EH5xdxwdr7zJWo4aBvzmRBMRmmRdzrd8xBzHbqxQlMhEp7E3S4sDHw5sbG5zaE0rFu E0OVRhTTJa4CpgVpd7vff56EQe2iJl2zVwVDmQ7g= Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 BF97C6EC59 for ; Wed, 10 Mar 2021 20:38:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BF97C6EC59 Received: by smtp47.i.mail.ru with esmtpa (envelope-from ) id 1lK2nI-0008Mv-NK; Wed, 10 Mar 2021 20:38:49 +0300 Date: Wed, 10 Mar 2021 20:37:59 +0300 To: Sergey Ostanevich Message-ID: <20210310173759.GI6842@root> References: <20210309175422.25432-1-skaplun@tarantool.org> <51AE3320-136E-422E-9BCC-AFA4A338B77A@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <51AE3320-136E-422E-9BCC-AFA4A338B77A@tarantool.org> X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9D3134714A9BDB69B7D7089AC2F541D13D72A77FF59123A9B00894C459B0CD1B941F6409E046689F9E5DEF55D21A193DBEE27ED51C03DAE4576BFA05F37B44EAE X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE74BE895B46187343CEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063748C26B83F2B024408638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C11F5982A89E0728C1738AE182D187F5DA70F7F169DF0CB64A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C317B107DEF921CE79117882F4460429728AD0CFFFB425014E868A13BD56FB6657A7F4EDE966BC389F9E8FC8737B5C2249AC294AFEFA671E80089D37D7C0E48F6CCF19DD082D7633A0E7DDDDC251EA7DABAAAE862A0553A39223F8577A6DFFEA7C33389216BB544DE543847C11F186F3C5E7DDDDC251EA7DABCC89B49CDF41148FA8EF81845B15A4842623479134186CDE6BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5AB1B449D5FA2D1DDF79BA95CBF9C738F04563E2E6BDAB64ED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75F04B387B5D7535DE410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34023CA4E4726A7D6C1E11091BE0F46F045BDFDBD3B323827C334930FBA045E5A8FCEDB1333059EEA31D7E09C32AA3244C9B182985D504D804BE9BE15671ADD91B55E75C8D0ED9F6EEFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojsR8tyFmO15PiiVKpVCpfdA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB4CC8B9100549CEDB757F64A1419700CADB4B1E5B0E6359966F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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" Hi, Sergos! Thanks for the review! On 10.03.21, Sergey Ostanevich wrote: > Hi! > > Thanks for the patch! > > > On 9 Mar 2021, at 20:54, Sergey Kaplun wrote: > > > > Resizing of the Lua stack is not reported as internal allocation. > > Moreover, it may lead to crash inside Lua or FF frames. > > > > Profiler performs reallocation first and after reports corresponding > > event. When the stack is resized for local function arguments, the link > > to previous frame is invalid in the cause of reallocation. Therefore, > ^^^^^^^^ case > > assertion in `debug_framepc()` failes, because of invalid function > fails. ^^^ is it just dubbing the link > is invalid? Then just remove. Fixed and force-pushed to the branch. The commit message now is the following: =================================================================== memprof: report stack resizing as internal event Resizing of the Lua stack is not reported as internal allocation. Moreover, it may lead to crash inside Lua or FF frames. Profiler performs reallocation first and after reports corresponding event. When the stack is resized for local function arguments, the link to previous frame is invalid in case of reallocation. Therefore, assertion in `debug_framepc()` fails. Resolves tarantool/tarantool#5842 Follows up tarantool/tarantool#5442 =================================================================== > > > reference at previous frame. > > > > 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; > > + > > lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1); > > + > > + setvmstate(G(L), INTERP); > > This looks like a hack. But even so - why not to return it back right after the > realloc, where you supposedly use lua_getinfo? The Lua state L is inconsistent during stack resizing, so it can't be interpreted in any way, until reallocation and corresponding converting are finished. > > The righteous way would be to fix the debug machinery anyhow. Don't get your point here. Lua state is invalid, debug machinery works correct, as I understand. > > > 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. > We -> ourselves? I've removed confusing "itself". See the iterative patch below, branch is force-pushed. =================================================================== diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua index 93cc348..9b9fb76 100644 --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua @@ -126,7 +126,7 @@ test:ok(check_alloc_report(alloc, 25, 18, 100)) test:ok(free.INTERNAL.num == 102) -- Test for https://github.com/tarantool/tarantool/issues/5842. --- We do not interested in report itself. +-- We do not interested in this report. 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). =================================================================== > > +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) > > +local co = coroutine.create(f) > > +coroutine.resume(co) > > +misc.memprof.stop() > > + > > jit.on() > > os.exit(test:check() and 0 or 1) > > — > > 2.28.0 > > > > > Regards, > Sergos > > -- Best regards, Sergey Kaplun