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 659E96E459; Wed, 17 Nov 2021 11:19:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 659E96E459 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1637137165; bh=TxyyHuXM7M9vqKHQJKqm2QlR6+Oun0hXzyp0vXCMpmU=; 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=Tnr7cg/zJN0dx/rI5F7UiA8NDb1sD8IuvWfggBlNc32gKs05SPDLpi7oP+x97gwi9 RQ1cioW4QKFV6rzvVj9ngKBFYhnGyQzfxgNIQL334ZnVeSF+F718F/AR3xHDhyKx6D Yrz8P4pGGepJSdHKnu3KghH6Cm2qqyD9ipwv/Y7Y= Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 3F5C76E459 for ; Wed, 17 Nov 2021 11:19:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3F5C76E459 Received: by smtp32.i.mail.ru with esmtpa (envelope-from ) id 1mnGA7-0005l0-FI; Wed, 17 Nov 2021 11:19:23 +0300 Date: Wed, 17 Nov 2021 11:17:33 +0300 To: Igor Munkin 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=us-ascii Content-Disposition: inline In-Reply-To: X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9731B3922EC063979DD5E01717F989B5F47BAD767D6B13F3A00894C459B0CD1B98C9CD1A2CF3AA9381C24B1D328D700291A9917A6B3250F1EB939BEC6D9612482 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73B2A9F8A35432468EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A164EE347039141E8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8D1805013C2F787021A8779662B6A13C6117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6AC294AFEFA671E80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A549AB5C7B365981903F3D23A7478C5B3147C350C5D6072177D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7506FE1F977233B9BB410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34A150564C13026C186CDB59BA8DDAF089B3A52E658746DD6A747374EEF14AA6E0A5CD0EDB920797DD1D7E09C32AA3244C9822E9530303EB8185CB2697DDEB3A9C81560E2432555DBBFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj81eoY04PbomdW6s2EtSwEA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB47883B1A7C168643FABDE61EC3C5B275215E412F7CDCFEC5EF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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: 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, Mikhail, Sorry, I don't see the letter to which Igor answered, so I reply here. LGTM, after fixes mentioned by Igor. On 12.11.21, Igor Munkin wrote: > 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. OK, if we can interpret this as the token to parser for our output, I'm totally fine to leave it in the old place. > > > > > 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 -- Best regards, Sergey Kaplun