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 A524A6FC82; Fri, 12 Nov 2021 16:34:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A524A6FC82 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1636724070; bh=BN2mcJ5BCd72eMK3kl8SVKe6U0xS0u+kpheF6/ddOWM=; 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=hanSOIEXAUwu3uOUFt0PyiEPVZc6cxMxMeJGHKEfBfx6FA2UO6n2Zn6YX8AH50V25 aigF2FQb/YBYRT2umW6Nj9AafW0Hc7NcgOXxlcT9IPk1xdmwgwhI+AVlrn7/wz4As4 Mbyytp1bdqV1q0WwUa1Ds+6mBIgIqsV4I9eqlhbk= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [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 A709D6FC82 for ; Fri, 12 Nov 2021 16:34:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A709D6FC82 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mlWhI-0000GG-Fp; Fri, 12 Nov 2021 16:34:29 +0300 Date: Fri, 12 Nov 2021 16:34:21 +0300 To: Mikhail Shishatskiy Message-ID: References: <20210929200758.149446-1-m.shishatskiy@tarantool.org> <20210929200758.149446-5-m.shishatskiy@tarantool.org> <20211101163113.GE8831@tarantool.org> <20211104130228.x6qcne5xeh544hm7@surf.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20211104130228.x6qcne5xeh544hm7@surf.localdomain> X-Clacks-Overhead: GNU Terry Pratchett X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9731B3922EC063979CAB00300CDA98DC51CCA794E1CC6B18A00894C459B0CD1B9542D81A8DEE81E006E7A41F49FED5FDD8542DB4064EE1F61467020CA3B4CCCD9 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B8498AC83B273C12EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006377A9AC615CFEE341F8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8E5EADF6F05F6E64010735C8CF65B9339117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BF1175FABE1C0F9B6A471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CC415C329B279CF9D43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A51A15DFC7B77532A9897EAC267DF119CB7BFE2DA0AA610C55D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7536C62C4FBC402878410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D345C110A855FC099990E140B2A32C409773E44B310B0300F665999882AB3028D3F9FF56064A3CF4D0F1D7E09C32AA3244C2A07B7B8E306B84E31B336A6B2B8371D8894E9C85370243E927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojLk+X/GdHsnrm8prYUjrggQ== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D4C6B3822AF9CD62B0392EDCB954F84A5A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v4 4/4] memprof: add info about trace start to symtab 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" Misha, Thanks for your fixes! Please consider my answer regarding trace event rendering and a typo in the comment below. On 04.11.21, Mikhail Shishatskiy wrote: > Hi, Igor! > Thank you for the review. > > New commit message: > ============================================================ > memprof: add info about trace start to symtab > > Allocation events occured on traces are recorded by the memory > profiler the following way now > > | TRACE [] > > This approach is not descriptive enough to understand, where > exactly allocation took place, as we do not know the code > chunk, associated with the trace. > > This patch fixes the problem described above by extending the > symbol table with entries, consisting of a trace's > mcode starting address, trace number, address of function proto, > and line, where trace recording started: > > | sym-trace := sym-header trace-no trace-addr sym-addr sym-line > | trace-no := > | trace-addr := > > The memory profiler parser is adjusted to recognize the entries > mentioned above. On top of that, the API of changed: > now table with symbols contains two tables: `lfunc` for Lua functions > symbols and `trace` for trace entries. > > The demangler module has not changed, but the function > `describe_location` is added to the module, > which allows one to get a description of the trace location in the > format described below: > > | TRACE [] started at @: > > Follows up tarantool/tarantool#5814 > ============================================================ > > Diff: > ============================================================ > diff --git a/src/lj_memprof.c b/src/lj_memprof.c > index e8b2ebbc..05542052 100644 > --- a/src/lj_memprof.c > +++ b/src/lj_memprof.c > @@ -40,7 +40,6 @@ static void dump_symtab_trace(struct lj_wbuf *out, const GCtrace *trace) > startpc < proto_bc(pt) + pt->sizebc); > > lineno = lj_debug_line(pt, proto_bcpos(pt, startpc)); > - lua_assert(lineno >= 0); > > lj_wbuf_addbyte(out, SYMTAB_TRACE); > lj_wbuf_addu64(out, (uint64_t)trace->traceno); > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua > index 3003b9f5..ce667afc 100644 > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua > @@ -92,7 +92,7 @@ local function fill_ev_type(events, symbols, event_type) > addr = trace_loc.addr > ev_type.trace[traceno] = { > name = string.format("TRACE [%d] %s:%d", > - traceno, symbols.lfunc[addr].source, symbols.lfunc[addr].linedefined > + traceno, symbols.lfunc[addr].source, trace_loc.line > ), > num = event.num, > } > @@ -122,7 +122,7 @@ local function check_alloc_report(alloc, location, nevents) > local traceno = location.traceno > if traceno then > expected_name = string.format("TRACE [%d] ", traceno).. > - form_source_line(location.linedefined) > + form_source_line(location.line) > event = alloc.trace[traceno] > else > expected_name = form_source_line(location.linedefined) > @@ -244,9 +244,7 @@ test:test("jit-output", function(subtest) > local free = fill_ev_type(events, symbols, "free") > > -- We expect, that loop will be compiled into a trace. > - subtest:ok(check_alloc_report( > - alloc, { traceno = 1, line = 37, linedefined = 32 }, 20 > - )) Side note: I see there is neither of line and linedefined in the previous patch, so everything is OK. > + subtest:ok(check_alloc_report(alloc, { traceno = 1, line = 37 }, 20)) > -- See same checks with jit.off(). > subtest:ok(check_alloc_report(alloc, { line = 34, linedefined = 32 }, 2)) > subtest:ok(free.INTERNAL.num == 22) > diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua > index 7d30f976..caab8b3a 100644 > --- a/tools/memprof/humanize.lua > +++ b/tools/memprof/humanize.lua > @@ -7,7 +7,7 @@ local symtab = require "utils.symtab" > > local M = {} > > -function M.describe_location(symbols, loc) > +local function describe_location(symbols, loc) > if loc.traceno == 0 then > return symtab.demangle(symbols, loc) > end > @@ -15,7 +15,7 @@ function M.describe_location(symbols, loc) > local trace = symbols.trace[loc.traceno] > > -- If trace, which was remembered in the symtab, has not > - -- been flushed, assotiate it with a proto, where trace > + -- been flushed, associate it with a proto, where trace > -- recording started. > if trace and trace.addr == loc.addr then > return symtab.demangle(symbols, loc).." started at ".. > @@ -38,7 +38,7 @@ function M.render(events, symbols) > for i = 1, #ids do > local event = events[ids[i]] > print(string.format("%s: %d events\t+%d bytes\t-%d bytes", > - M.describe_location(symbols, event.loc), > + describe_location(symbols, event.loc), > event.num, > event.alloc, > event.free > @@ -46,7 +46,7 @@ function M.render(events, symbols) > > local prim_loc = {} > for _, heap_chunk in pairs(event.primary) do > - table.insert(prim_loc, M.describe_location(symbols, heap_chunk.loc)) > + table.insert(prim_loc, describe_location(symbols, heap_chunk.loc)) > end > if #prim_loc ~= 0 then > table.sort(prim_loc) > @@ -80,7 +80,7 @@ function M.leak_info(dheap, symbols) > -- with enabled jit. > if info.dbytes > 0 then > table.insert(leaks, { > - line = M.describe_location(symbols, info.loc), > + line = describe_location(symbols, info.loc), > dbytes = info.dbytes > }) > end > diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua > index 49ebb36f..879979f8 100644 > --- a/tools/utils/symtab.lua > +++ b/tools/utils/symtab.lua > @@ -39,6 +39,9 @@ local function parse_sym_trace(reader, symtab) > > symtab.trace[traceno] = { > addr = trace_addr, > + -- The structure is the same as the one > + -- yielded from the fucntion Typo: s/fucntion/function/. > + -- in the module. > start = { > addr = sym_addr, > line = sym_line, > ============================================================ > > Issue: https://github.com/tarantool/tarantool/issues/5814 > Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number > CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number > > On 01.11.2021 19:31, Igor Munkin wrote: > >Misha, > > > >Thanks for the patch! Please consider my comments below. > > > >On 29.09.21, Mikhail Shishatskiy wrote: > >> Trace allocation sources, recorded by the memory profiler, > >> were reported as > > > > >> > >> | TRACE [] > >> > >> This approach is not descriptive enough to understand, where > >> exactly allocation took place, as we do not know the code > >> chunk, associated with the trace. > >> > >> This patch fixes the problem described above by extending the > >> symbol table with entries, consisting of a trace's > >> mcode starting address, trace number, address of function proto, > >> and line, where trace recording started: > >> > >> | sym-trace := sym-header trace-no trace-addr sym-addr sym-line > >> | trace-no := > >> | trace-addr := > >> > >> The memory profiler parser is adjusted to recognize the entries > >> mentioned above. On top of that, the API of changed: > >> now table with symbols contains two tables: `lfunc` for Lua functions > >> symbols and `trace` for trace entries. > >> > >> The demangler module has not changed, but the function > >> `describe_location` is added to the module, > >> which allows one to get a description of the trace location in the > >> format described below: > >> > >> | TRACE [] started at @: > >> > >> Follows up tarantool/tarantool#5814 > >> --- > >> > >> Issue: https://github.com/tarantool/tarantool/issues/5814 > >> Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number > >> CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number > >> > >> src/lj_memprof.c | 43 +++++++++++++++++++ > >> src/lj_memprof.h | 8 +++- > >> .../misclib-memprof-lapi.test.lua | 15 ++++--- > >> tools/memprof.lua | 4 +- > >> tools/memprof/humanize.lua | 30 ++++++++++--- > >> tools/memprof/process.lua | 9 ++-- > >> tools/utils/symtab.lua | 31 ++++++++++--- > >> 7 files changed, 118 insertions(+), 22 deletions(-) > >> > >> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua > >> index 3f4ffea0..b9edb80d 100644 > >> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua > >> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua > >> @@ -116,7 +120,8 @@ end > >> local function check_alloc_report(alloc, traceno, line, function_line, nevents) > >> local expected_name, event > >> if traceno ~= 0 then > >> - expected_name = string.format("TRACE [%d]", traceno) > >> + expected_name = string.format("TRACE [%d] ", traceno).. > >> + form_source_line(function_line) > > > >The output format differs from the one produced by memprof parser, > >doesn't it? > > Yes, because we demangle names in function. So, we can > omit "started at" part to check that everything else is correct. Oh, I see... This is odd a bit but now I got it, thanks! > > >> diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua > >> index 7771005d..7d30f976 100644 > >> --- a/tools/memprof/humanize.lua > >> +++ b/tools/memprof/humanize.lua > >> @@ -7,6 +7,23 @@ local symtab = require "utils.symtab" > >> + -- recording started. > >> + if trace and trace.addr == loc.addr then > >> + return symtab.demangle(symbols, loc).." started at ".. > >> + symtab.demangle(symbols, trace.start) > > > >Finally, I got the thing that bothers me the most. Why do you make > > so complex? It looks that you can move all these > >if-else branching to and concatenation to > > function, doesn't it? AFAICS, you can remove > > as a result and trace demangling will be > >encapsulated in scope of function. Feel free to correct > >me if I'm wrong. > > Initially it was implemented, as you suggest now. But Sergey in his > review led me to believe, that "started at" part should ideologically > relate to the humanizer module. And I agree with that point, but maybe > I decomposed things not in a very good way. Em... In that way all other types (such as "INTERNAL" and "CFUNC %#x") should also be in the humanizer module, since this representation is specific for a particular output format. All in all nobody stops you from moving to the humanize module, since it's used only there (and need to be used only there). BTW, Sergey is also in Cc, so he can also drop a few words regarding it. > > Another way to implement this is to demangle without "started at" and > then insert it to the demangled name. What do you think? My point is to have the whole "stringification" mess encapsulated in a single function (like it's almost done within ). And the only thing remaining outside of this function is "started at" tail. I hope this fits your vision regarding decomposition :) > > > > >> + end > >> + return symtab.demangle(symbols, loc) > >> +end > >> + > >> -- > >> 2.33.0 > >> > > > >-- > >Best regards, > >IM > > -- > Best regards, > Mikhail Shishatskiy -- Best regards, IM