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 7D2816EC55; Tue, 27 Jul 2021 17:00:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7D2816EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627394448; bh=iFMUbQ3joEfOdnbDTx4LyTRELlnj8LKt93gqDZxQLP8=; 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=jC4uJAlhMBbcRvITjt5trTWazLAwTWhU/tLE9F1AnuQscEfi1WxqSKSY3wAkaRH7I zkk67fc42v11Gtwd5Ra4f1lxYVhc3kLZvZFQT4N4JM5sNuyJDroFtikPdW+0JUKYw/ s+33oi912fR0qSi8e03PswJTkCu5+UL1OY+ljVRI= Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 E48116EC55 for ; Tue, 27 Jul 2021 17:00:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E48116EC55 Received: by smtp41.i.mail.ru with esmtpa (envelope-from ) id 1m8NdX-0008R6-1Y; Tue, 27 Jul 2021 17:00:47 +0300 Date: Tue, 27 Jul 2021 16:59:35 +0300 To: Mikhail Shishatskiy Message-ID: References: <20210722114617.194747-1-m.shishatskiy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210722114617.194747-1-m.shishatskiy@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C3104FC76DFAAAAF7DA068FE323FAC4379182A05F538085040E5A73E52D8EC63F7A6D9B96DB8C7A0EC2F3503F0EF6C540A7034BA54C00F9774 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE737BB76880A4CA9A4EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378ABD31E9FF1CD53C8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8219932B035D17795013EA29914529C89117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC8C7ADC89C2F0B2A5A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B65D56369A3576CBA5089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5C2ACC3657DF4A4F71A9C5C81624AF8957CDCC6B3B8C11623D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75FA7FF33AA1A4D21C410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C786159FDC4342B0C9CC697682BB794EF07038D3BD1C179065C5073AB9B7FAE7204C7F03273F63C21D7E09C32AA3244C7F0D43A28A88342980B68719E1479F589CA7333006C390A0FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojMEANdStWW5844HsIEBlq6g== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB44E3744646BF7B9F6C4DE97D7376B78E236F306002D5A2ECFF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v1] memprof: report all JIT-related allocations as internal 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, Mikhail! Thanks for the patch! On 22.07.21, Mikhail Shishatskiy wrote: | memprof: report all JIT-related allocations as internal Width limit for commit title is 50 symbols including subsystem name considering our code style guide [1]. > There are cases when the memory profiler attempts to attribute > allocations triggered by the JIT engine recording phase > with a Lua function to be recorded. In this case, > lj_debug_frameline() may return BC_NOPOS (i.e. a negative value). > > Previously, these situations were ignored and the profiler > reported, that the source line was equal to zero. > > This patch adjusts profiler behavior to treat allocations > described above as internal by dumping ASOURCE_INT if > lj_debug_frameline() returns a negative value. Maybe it is better to catch out the JIT-related allocation and associate it with the corresponding trace after these issues [2][3] will be resolved, isn't it? Otherwise after fixing other nitpicks patch LGTM. Thoughts? > > Resolves tarantool/tarantool#5679 > --- > > Issue: https://github.com/tarantool/tarantool/issues/5679 > Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal Sorry, I can't find Tarantool's standalone branch with these changes to check CI. May you please share it here? > > src/lj_memprof.c | 28 +++++++---- > .../misclib-memprof-lapi.test.lua | 50 ++++++++++++------- > 2 files changed, 51 insertions(+), 27 deletions(-) > > diff --git a/src/lj_memprof.c b/src/lj_memprof.c > index 2c1ef3b8..b4985b8e 100644 > --- a/src/lj_memprof.c > +++ b/src/lj_memprof.c > @@ -89,19 +89,27 @@ static void memprof_write_lfunc(struct lj_wbuf *out, uint8_t aevent, > GCfunc *fn, struct lua_State *L, > cTValue *nextframe) > { > - const BCLine line = lj_debug_frameline(L, fn, nextframe); > - lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC); > - lj_wbuf_addu64(out, (uintptr_t)funcproto(fn)); > /* > - ** Line is >= 0 if we are inside a Lua function. > - ** There are cases when the memory profiler attempts > - ** to attribute allocations triggered by JIT engine recording > - ** phase with a Lua function to be recorded. At this case > - ** lj_debug_frameline() may return BC_NOPOS (i.e. a negative value). > - ** Equals to zero when LuaJIT is built with the > + ** Line equals to zero when LuaJIT is built with the > ** -DLUAJIT_DISABLE_DEBUGINFO flag. > */ > - lj_wbuf_addu64(out, line >= 0 ? (uint64_t)line : 0); > + const BCLine line = lj_debug_frameline(L, fn, nextframe); > + > + if (line < 0) { > + /* > + ** Line is >= 0 if we are inside a Lua function. > + ** There are cases when the memory profiler attempts > + ** to attribute allocations triggered by JIT engine recording > + ** phase with a Lua function to be recorded. It this case, > + ** lj_debug_frameline() may return BC_NOPOS (i.e. a negative value). > + ** We report such allocations as internal in order not to confuse users. > + */ > + lj_wbuf_addbyte(out, aevent | ASOURCE_INT); > + } else { > + lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC); > + lj_wbuf_addu64(out, (uintptr_t)funcproto(fn)); > + lj_wbuf_addu64(out, (uint64_t)line); > + } > } > > static void memprof_write_cfunc(struct lj_wbuf *out, uint8_t aevent, > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua > index 06d96b3b..e35d8c29 100644 > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua > @@ -7,7 +7,7 @@ require("utils").skipcond( > local tap = require("tap") > > local test = tap.test("misc-memprof-lapi") > -test:plan(13) > +test:plan(14) > > jit.off() > jit.flush() > @@ -22,7 +22,7 @@ local symtab = require "utils.symtab" > local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.memprofdata.tmp.bin") > local BAD_PATH = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%1/memprofdata.tmp.bin") > > -local function payload() > +local function default_payload() > -- Preallocate table to avoid table array part reallocations. > local _ = table_new(100, 0) > > @@ -37,7 +37,7 @@ local function payload() > collectgarbage() > end > > -local function generate_output(filename) > +local function generate_output(filename, payload) > -- Clean up all garbage to avoid pollution of free. > collectgarbage() > > @@ -52,6 +52,25 @@ local function generate_output(filename) > assert(res, err) > end > > +local function generate_parsed_output(filename, payload) > + local res, err = pcall(generate_output, filename, payload) > + > + -- Want to cleanup carefully if something went wrong. > + if not res then > + os.remove(filename) > + error(err) > + end > + > + local reader = bufread.new(filename) > + local symbols = symtab.parse(reader) > + local events = memprof.parse(reader) > + > + -- We don't need it any more. > + os.remove(filename) > + > + return symbols, events > +end > + > local function fill_ev_type(events, symbols, event_type) > local ev_type = {} > for _, event in pairs(events[event_type]) do > @@ -107,20 +126,7 @@ test:ok(res == nil and err:match("profiler is not running")) > test:ok(type(errno) == "number") > > -- Test profiler output and parse. > -res, err = pcall(generate_output, TMP_BINFILE) > - > --- Want to cleanup carefully if something went wrong. > -if not res then > - os.remove(TMP_BINFILE) > - error(err) > -end > - > -local reader = bufread.new(TMP_BINFILE) > -local symbols = symtab.parse(reader) > -local events = memprof.parse(reader, symbols) > - > --- We don't need it any more. > -os.remove(TMP_BINFILE) > +local symbols, events = generate_parsed_output(TMP_BINFILE, default_payload) > > local alloc = fill_ev_type(events, symbols, "alloc") > local free = fill_ev_type(events, symbols, "free") > @@ -166,5 +172,15 @@ local co = coroutine.create(f) > coroutine.resume(co) > misc.memprof.stop() > > +-- Test for marking jit-related allocations as internal. > +-- See also https://github.com/tarantool/tarantool/issues/5679. > jit.on() > +symbols, events = generate_parsed_output(TMP_BINFILE, function() Why do you use this instead of default_payload? Please drop a comment here. > + for _ = 1, 100 do > + local _ = {_, _} > + end > +end) > +alloc = fill_ev_type(events, symbols, "alloc") > +test:ok(alloc[0] == nil) > + > os.exit(test:check() and 0 or 1) > -- > 2.32.0 > [1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message [2]: https://github.com/tarantool/tarantool/issues/5814 [3]: https://github.com/tarantool/tarantool/issues/5815 -- Best regards, Sergey Kaplun