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 ACB7D6FC86; Thu, 16 Sep 2021 18:58:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org ACB7D6FC86 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1631807939; bh=3m04ht/RP8BsksX2s4K92KKPZQZIPryJO7VIKSss7FU=; 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=f8WXz/GPC1Y3YMiJ6CdNuQUF/C46lbx5/CZOnlW3Uw3CQKcp2PYfuuqBGaRcMXKqu 8Q1vkROitSSxsoPvXdBVkG0WWPXK38EluKYg3FMQp/nu4DoZDbk8MsHZ3g2pyStO1o yB+yhJXv1mJli29p8R08UzyBtEFz3nvdyPoAkrSc= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [94.100.179.3]) (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 065AB6FC86 for ; Thu, 16 Sep 2021 18:58:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 065AB6FC86 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1mQtmr-00058T-08; Thu, 16 Sep 2021 18:58:57 +0300 Date: Thu, 16 Sep 2021 18:32:43 +0300 To: Mikhail Shishatskiy Message-ID: <20210916153243.GD6844@tarantool.org> References: <20210820070546.115293-1-m.shishatskiy@tarantool.org> <20210820070546.115293-5-m.shishatskiy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210820070546.115293-5-m.shishatskiy@tarantool.org> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD91AE02D33A9C88A2FBA0FFC16CEFBFC694531AE86BFF036DF00894C459B0CD1B93E78936D346A28472D83964825D0E3C62865F1F1D023BD9567D05D8178A78D18 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72AC9FB60380F23AEEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637993F4D087AF024C68638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8C143C71EF4580AAB95A7FBAB92C7CEBF117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18C26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6D635BA3ABDB36C18089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5055428A3E17DA3992965B6A32AB392A5D5 X-C1DE0DAB: 0D63561A33F958A547EC0CAACF0A4931C341C3F46B02713D42725D01784EE888D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7540E9CF2C1C1CEBBA410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D346BEB6F73FB9D1E4A3C37327CD5EBBBC1695D3BD424512C382D0235EB6330FF24CA8918FD8562A6361D7E09C32AA3244C0EBD644D9EC23BAAC8C29A1C71445EEA3C6EB905E3A8056B927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojbgLrBZmQxDSOlIUSLyyiFw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DF9D3DA3EB94D1F016A86B01DC721898CA7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v3 4/5] memprof: extend symtab with info about traces 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 the patch! Why don't you squash this one patch with the previous one? What is the rationale to split these changes? On 20.08.21, Mikhail Shishatskiy wrote: > This patch extends the memprof symtab by adding information > about traces, which are present at the start of profiling. > > The symtab format changed with adding entry, > which consists of a trace address, trace number, address > of function proto, and line, where trace recording started: Again, traceno should be the first, IMHO. > > | sym-trace := sym-header trace-addr trace-no sym-addr sym-line > | trace-addr := > | trace-no := > > Also, the memprof parser is adjusted to recognize new > symtab entries and associate them with allocation events > from traces. > > With adding traces to the symbol table the API of > changed: now table with symbols contains two tables: `lfunc` for > Lua functions symbols and `trace` for trace entries. > > As trace can be associated with a function proto, > module extended with a function `describe_location` intended > to be used instead of symtab.demangle. The reason is that the > location name can be more than demangled `loc`: for example, > when getting trace location name, we want to assotiate the > trace with a function proto, where trace recording started. Ouch, too tricky description. I guess you mean that ev_line can represent both allocations in VM and on trace, right? > On the one hand, we want to get a verbose trace description > with demangled proto location: > > | TRACE [] started at @: > > On the other hand, idiomatically, this should be done not by > the demangler module but by the humanizer module. As we discussed offline, you violate so-called MVVM or MVC patterns (I might be wrong in terms, so excuse me, if I am): none of the parser modules can depend on humanize.lua -- this is the top-most part of the parser, that can produce plain text, JSON, CSV or anything else. See more comments below. > > Resolves tarantool/tarantool#5814 > --- > > Issue: https://github.com/tarantool/tarantool/issues/5814 > Luajit branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number > tarantool branch: https://github.com/tarantool/tarantool/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number > > src/lj_memprof.c | 40 ++++++ > src/lj_memprof.h | 7 +- > .../misclib-memprof-lapi.test.lua | 135 +++++++++++++----- > tools/memprof/humanize.lua | 21 ++- > tools/memprof/process.lua | 6 +- > tools/utils/symtab.lua | 45 ++++-- > 6 files changed, 206 insertions(+), 48 deletions(-) > > diff --git a/src/lj_memprof.c b/src/lj_memprof.c > index fb99829d..fc5bc301 100644 > --- a/src/lj_memprof.c > +++ b/src/lj_memprof.c > @@ -28,6 +28,42 @@ > static const unsigned char ljs_header[] = {'l', 'j', 's', LJS_CURRENT_VERSION, > 0x0, 0x0, 0x0}; > > +#if LJ_HASJIT > + > +static void dump_symtab_trace(struct lj_wbuf *out, const GCtrace *trace) > +{ > + const GCproto *pt = &gcref(trace->startpt)->pt; > + BCLine lineno = -1; Why do you use -1 here? > + > + const BCIns *startpc = mref(trace->startpc, const BCIns); > + lua_assert(startpc >= proto_bc(pt) && > + 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->mcode); > + lj_wbuf_addu64(out, (uint64_t)trace->traceno); > + /* > + ** All the existing prototypes have already been dumped, so we do not > + ** need to repeat their dump for trace locations. Do you mean, that we don't need to dump chunk name here again? It's also worth to mention, that GCproto is anchored via GCtrace, so it's not collected until the trace is alive. > + */ > + lj_wbuf_addu64(out, (uintptr_t)pt); > + lj_wbuf_addu64(out, (uint64_t)lineno); > +} > @@ -47,6 +83,10 @@ static void dump_symtab(struct lj_wbuf *out, const struct global_State *g) > lj_wbuf_addu64(out, (uint64_t)pt->firstline); > break; > } > + case (~LJ_TTRACE): { > + dump_symtab_trace(out, gco2trace(o)); Minor: IMHO, it's better to put the function body right here (like it's done for LJ_TPROTO), but feel free to ignore. > + break; > + } > default: > break; > } > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua > index dbf384ed..f84b6df0 100644 > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua > @@ -52,19 +59,49 @@ local function generate_output(filename) > assert(res, err) > end > > +local function generate_parsed_output(payload) Minor: It's better to introduce this in the second patch, IMHO. Feel free to ignore. > + local res, err = pcall(generate_output, TMP_BINFILE, payload) > + > + -- 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) > + > + -- We don't need it any more. > + os.remove(TMP_BINFILE) > + > + return symbols, events > +end > + > local function fill_ev_type(events, symbols, event_type) > local ev_type = {} > for _, event in pairs(events[event_type]) do > local addr = event.loc.addr > - if addr == 0 then > + local traceno = event.loc.traceno > + > + if traceno ~= 0 and symbols.trace[traceno] then > + local trace_loc = symbols.trace[traceno].sym_loc > + addr = trace_loc.addr > + ev_type[trace_loc.line] = { > + name = string.format("TRACE [%d] %s:%d", > + traceno, symbols.lfunc[addr].source, symbols.lfunc[addr].linedefined > + ), > + num = event.num, > + } > + elseif addr == 0 then > ev_type.INTERNAL = { > name = "INTERNAL", > num = event.num, > } Hm. Looks like a misindent in the old code. > - elseif symbols[addr] then > + elseif symbols.lfunc[addr] then > ev_type[event.loc.line] = { > name = string.format( > - "%s:%d", symbols[addr].source, symbols[addr].linedefined > + "%s:%d", symbols.lfunc[addr].source, symbols.lfunc[addr].linedefined > ), > num = event.num, > } > @@ -179,5 +215,38 @@ test:test("stack-resize", function(subtest) > misc.memprof.stop() > end) > > +-- Test profiler with enabled JIT. > jit.on() > + > +test:test("jit-output", function(subtest) > + -- Disabled on *BSD due to #4819. > + if jit.os == 'BSD' then > + subtest:plan(1) This line is excess, since you use skip below. > + subtest:skip('Disabled due to #4819') > + return > + end > + > + subtest:plan(3) > + > + jit.opt.start(3, "hotloop=10") > + jit.flush() > + > + -- Pregenerate traces to fill symtab entries in the next run. > + default_payload() > + > + local symbols, events = generate_parsed_output(default_payload) > + > + local alloc = fill_ev_type(events, symbols, "alloc") > + local free = fill_ev_type(events, symbols, "free") > + > + -- We expect, that loop will be compiled into a trace. > + subtest:ok(check_alloc_report(alloc, 1, 37, 32, 20)) > + -- See same checks with jit.off(). > + subtest:ok(check_alloc_report(alloc, 0, 34, 32, 2)) > + subtest:ok(free.INTERNAL.num == 22) > + > + -- Restore default JIT settings. > + jit.opt.start(unpack(jit_opt_default)) > +end) > + > os.exit(test:check() and 0 or 1) > diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua > index 0bcb965b..f277ed84 100644 > --- a/tools/memprof/process.lua > +++ b/tools/memprof/process.lua > @@ -2,7 +2,7 @@ > > local M = {} > > -local symtab = require "utils.symtab" > +local humanize = require "memprof.humanize" > > function M.form_heap_delta(events, symbols) > -- Auto resurrects source event lines for counting/reporting. > @@ -17,7 +17,7 @@ function M.form_heap_delta(events, symbols) > > for _, event in pairs(events.alloc) do > if event.loc then > - local ev_line = symtab.demangle(symbols, event.loc) > + local ev_line = humanize.describe_location(symbols, event.loc) You can use here function from the previous patch, as we discussed offline. As a result, you can just adjust and use it in humanize.lua. > > if (event.alloc > 0) then > dheap[ev_line].dbytes = dheap[ev_line].dbytes + event.alloc > @@ -37,7 +37,7 @@ function M.form_heap_delta(events, symbols) > -- that references the table with memory changed > -- (may be empty). > for _, heap_chunk in pairs(event.primary) do > - local ev_line = symtab.demangle(symbols, heap_chunk.loc) > + local ev_line = humanize.describe_location(symbols, heap_chunk.loc) > > if (heap_chunk.alloced > 0) then > dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.alloced > diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua > index 3ed1dd13..0e742ee1 100644 > --- a/tools/utils/symtab.lua > +++ b/tools/utils/symtab.lua > @@ -24,18 +25,38 @@ local function parse_sym_lfunc(reader, symtab) > local sym_chunk = reader:read_string() > local sym_line = reader:read_uleb128() > > - symtab[sym_addr] = { > + symtab.lfunc[sym_addr] = { > source = sym_chunk, > linedefined = sym_line, > } > end > > +local function parse_sym_trace(reader, symtab) > + local trace_addr = reader:read_uleb128() > + local traceno = reader:read_uleb128() > + local sym_addr = reader:read_uleb128() > + local sym_line = reader:read_uleb128() > + > + symtab.trace[traceno] = { > + addr = trace_addr, > + sym_loc = { I propose to rename to . > + addr = sym_addr, > + line = sym_line, It's better to use instead of here. > + traceno = 0, Why do you need this? > + }, > + } > +end > + > local parsers = { > [SYMTAB_LFUNC] = parse_sym_lfunc, > + [SYMTAB_TRACE] = parse_sym_trace, > } > > function M.parse(reader) > - local symtab = {} > + local symtab = { > + lfunc = {}, > + trace = {}, > + } > local magic = reader:read_octets(3) > local version = reader:read_octets(1) > > @@ -73,18 +94,26 @@ function M.parse(reader) > return symtab > end > > -function M.demangle(symtab, loc) > +local function demangle_lfunc(symtab, loc) > local addr = loc.addr > > if addr == 0 then > return "INTERNAL" > + elseif symtab.lfunc[addr] then > + return string_format("%s:%d", symtab.lfunc[loc.addr].source, loc.line) Why do you use here instead of ? > end > + return string_format("CFUNC %#x", addr) > +end > > - if symtab[addr] then > - return string_format("%s:%d", symtab[addr].source, loc.line) > - end > +local function demangle_trace(loc) > + return string_format("TRACE [%d] 0x%x", loc.traceno, loc.addr) > +end > > - return string_format("CFUNC %#x", addr) > +function M.demangle(symtab, loc) > + if loc.traceno ~= 0 then > + return demangle_trace(loc) > + end > + return demangle_lfunc(symtab, loc) > end > > return M > -- > 2.32.0 > -- Best regards, IM