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 001246FC87; Wed, 29 Sep 2021 22:22:00 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 001246FC87 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1632943321; bh=7Wy4ozON3LXya509UYUK1AjymrAnztoRMribj7lhwXM=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=CwHtaM163rJSjXqfSFzV/ydSLbsjdcpr1MZ8CLfpU8sqZsZnXllR7MnJHgW28dgWx q+R9iAD10AzsEVtcBjpPDdNvW7aP5kPZgQowPfP/hZ2ToLOGWU95uWYMWE+hX8yl9i nnl6dIXAr/aHEWe3esp2xVXCwQODs2ziPt8fu4mc= Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 E60056FC87 for ; Wed, 29 Sep 2021 22:21:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E60056FC87 Received: by smtp61.i.mail.ru with esmtpa (envelope-from ) id 1mVf9F-0001ht-2d; Wed, 29 Sep 2021 22:21:45 +0300 Date: Wed, 29 Sep 2021 22:21:43 +0300 To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Message-ID: <20210929192143.ojcgefqkpwritdk5@surf.localdomain> References: <20210820070546.115293-1-m.shishatskiy@tarantool.org> <20210820070546.115293-5-m.shishatskiy@tarantool.org> <20210916153243.GD6844@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Disposition: inline In-Reply-To: <20210916153243.GD6844@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD96A58C36AA2E99649BF631F26B0465AFD66E223DEB3A228DC182A05F538085040216F4374BB9A3B97D70F0FDA52CB0B7FC8CABC36147446B8D3917B8CB6DAA3C9 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7081BBE264C6D7F42EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B8A896DD3ADA3FA48638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8D95A8F7007860BE2B853D31D7FCBDA4A117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B8C7ADC89C2F0B2A5A471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FC68F5C563A1CB13B53AA81AA40904B5D9CF19DD082D7633A078D18283394535A93AA81AA40904B5D98AA50765F7900637825A57FC606BE619D81D268191BDAD3D698AB9A7B718F8C4D1B931868CE1C5781A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F83C798A30B85E16BCE5475246E174218B5C8C57E37DE458BEDA766A37F9254B7 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458F0AFF96BAACF4158235E5A14AD4A4A4625E192CAD1D9E79DB194B0D77246B867B88B1FE9DEE407BB X-C1DE0DAB: 0D63561A33F958A5849832C7F9489F6EFA3C0C8156B582A1DDA5E4277DF37C8DD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75BFC02AB3DF06BA5A410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3450C5E6D685282BA10D5433C2EE645DE36A0FFBDE30A82634FAF825343C58CFE2F83526F6DB28921E1D7E09C32AA3244C372DA5B7547EBA4CAE6CF5B8BA059B7BD9ADFF0C0BDB8D1F729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojsAIehEB+JdnDBu7kzZDedg== X-Mailru-Sender: EFA0F3A8419EF21635BFE795C6CB22C949F1B37477F6089ED70F0FDA52CB0B7FC9F871340C5A6C402376072A51849BFFE66B5C1DBFD5D09D5E022D45988A037B448E0EA96F20AB367402F9BA4338D657ED14614B50AE0675 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: Mikhail Shishatskiy via Tarantool-patches Reply-To: Mikhail Shishatskiy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Igor! Thank you for the review! On 16.09.2021 18:32, Igor Munkin wrote: >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? The upcoming patch series v4 fixes this. Now patches are split not by loading / rendering but by loading only basic info, simple rendering / extending symtab, descriptive rendering. > >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? Fixed in the upcoming patch series v4. > >> 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. Fixed in the upcoming patch series v4. > >> >> 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? Changed to 0 in order not to confuse. I believe that variable initialization is important. > >> + >> + 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. Yes, fixed the comment. > >> + */ >> + 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. But we need to leave the stub, when the JIT is disabled. Personally I do not like the defines right inside the function code. > >> + 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. Moved to the patch with test refactored. > >> + 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. The utils module uses this semantics, and if I omit the plan call, I will fail at the end of the test, as there is "no plan". > >> + 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) > > >> >> 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 . Fixed in the upcoming patch series v4. > >> + addr = sym_addr, >> + line = sym_line, > >It's better to use instead of here. Here I want to follow the semantics of the structure in order to pass it to the symtab.demangle. > >> + traceno = 0, > >Why do you need this? See explanation above (about ). > >> + }, >> + } >> +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 ? My bad, fixed. > >> 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 Best regards, Mikhail Shishatskiy