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 229DF6EC40; Sat, 14 Aug 2021 14:04:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 229DF6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628939070; bh=4FMZtJkk2CXA9PtiHVMc5cN70w9sR4uu4T4yvgtQnrU=; 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=WtX5DOKKvS1aLSdvdEOh1N96tbjxPJWZs9ciiL0zESCF+Cg2VV5KHmy9Fg+OtKLb5 61ejHQ3HKdnwYRJ4takL+Rkw4tG6elj1iBCTTzidFLjKWP/0zCHZ3XNXfGtJ0xwZ7m x1p5Cm1PLndIThzSHJgZ8/nBvi7lL564an1QlnW0= Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 828C26EC40 for ; Sat, 14 Aug 2021 14:04:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 828C26EC40 Received: by smtp59.i.mail.ru with esmtpa (envelope-from ) id 1mErSl-00050F-Gh; Sat, 14 Aug 2021 14:04:28 +0300 Date: Sat, 14 Aug 2021 14:03:11 +0300 To: Mikhail Shishatskiy Message-ID: References: <20210728134259.1113235-1-m.shishatskiy@tarantool.org> <20210728134259.1113235-3-m.shishatskiy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210728134259.1113235-3-m.shishatskiy@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD91BCCB18F2C129F87F36E61E9E4584E9D182A05F53808504078E3BC0A8627798444C7F27A38C06E1AB49A39A38C7E7BE14826B1DE57BE1282 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7590E57235B5C00BDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B4ADA525623CF2D68638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8271C5B69B32E4BB93D4509DE24C870D2117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735209ECD01F8117BC8BEA471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C468D16C903838CAB43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A541FAAC7108E9D589CF134B65FECDB678C9702FB89752AE8FD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752DA3D96DA0CEF5C48E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D349A949488F6BF46DDFDEC94997195A55C12F78B076727759B3116FA37EAB0DEC53C2B2B7EF06C68481D7E09C32AA3244CC202592622E5438EBD0E6D9A8858BED2C86C126E7119A0FEFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj57i8v0hCSFKt7QFGFvmSrA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB456335F0EDECA434151581B7EA65F70181BFC9F8C032416DCF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2 2/2] 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: 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! Please consider my comments below. On 28.07.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 number, address of function proto, > and line, where trace recording started. > > Also, the memprof parser was adjusted to recognize new > symtab entries and associate them with allocation events > from traces. > > This patch also provides tests to test the behavior of > the memory profiler with JIT turned on. Nit: This part is obvious and can be dropped, I suppose. Feel free to ignore. Also, please mention that API of is changed (I'm about trace\lfunc entries). > > Part of tarantool/tarantool#5814 Should it be Resolves here? > --- > > 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 | 19 +++ > src/lj_memprof.h | 7 +- > .../misclib-memprof-lapi.test.lua | 121 +++++++++++++----- > tools/utils/symtab.lua | 41 +++++- > 4 files changed, 150 insertions(+), 38 deletions(-) > > diff --git a/src/lj_memprof.c b/src/lj_memprof.c > index 0c7e9e89..87512c3a 100644 > --- a/src/lj_memprof.c > +++ b/src/lj_memprof.c > @@ -18,6 +18,7 @@ > #include "lj_obj.h" > #include "lj_frame.h" > #include "lj_debug.h" > +#include "lj_trace.h" I suppose you need "lj_jit.h" here, not "lj_trace.h". Also I suppose, that this should be enabled only, if LuaJIT compiled with enabled JIT. This regarding to the previous patch too -- we should add lua_assert(0); if LuaJIT compiled without JIT to all JIT-related functions (maybe in the separate commit). Also, please update Makefile.dep.original correspondingly with added header. > > /* --------------------------------- Symtab --------------------------------- */ > > @@ -43,6 +44,24 @@ 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): { > + GCtrace *trace = gco2trace(o); Nit: We don't want to change trace, so we can add `const` here > + GCproto *pt = &gcref(trace->startpt)->pt; Ditto. > + ^^^^^ Typo: Trailing whitespaces > + const BCIns *startpc = mref(trace->startpc, const BCIns); > + lua_assert(startpc >= proto_bc(pt) && Typo: Trailing whitespace --------------------^ > + startpc < proto_bc(pt) + pt->sizebc); > + > + BCLine lineno = lj_debug_line(pt, proto_bcpos(pt, startpc)); Nit: Please declare all variables at the beggining of the statement. Else you can observe the following warnings: | /home/burii/reviews/luajit/memprof-tracealloc/src/lj_memprof.c: In function 'dump_symtab': | /home/burii/reviews/luajit/memprof-tracealloc/src/lj_memprof.c:55:7: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] | 55 | BCLine lineno = lj_debug_line(pt, proto_bcpos(pt, startpc)); | | ^~~~~~ | /home/burii/reviews/luajit/memprof-tracealloc/src/lj_memprof.c: In function 'dump_symtab': | /home/burii/reviews/luajit/memprof-tracealloc/src/lj_memprof.c:55:7: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] | 55 | BCLine lineno = lj_debug_line(pt, proto_bcpos(pt, startpc)); if configure build like this: | cmake -DCMAKE_C_FLAGS="-Wextra -Wdeclaration-after-statement -Wredundant-decls -Wshadow -Wpointer-arith"\ | -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON . && make -j > + lua_assert(lineno >= 0); > + > + lj_wbuf_addbyte(out, SYMTAB_TRACE); > + lj_wbuf_addu64(out, (uint64_t)trace->traceno); > + lj_wbuf_addu64(out, (uintptr_t)pt); Nit: Please, add comment that all existing prototypes have already been dumped and we have no need to repeat their dump for trace locations. > + lj_wbuf_addu64(out, (uint64_t)lineno); > + > + break; ^ Typo: Trailling whitespace Side note: Please, tune your favorite editor to highlight trailing whitespaces. > + } > default: > break; > } > diff --git a/src/lj_memprof.h b/src/lj_memprof.h > index e3f55433..9e950b94 100644 > --- a/src/lj_memprof.h > +++ b/src/lj_memprof.h > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua > index 06d96b3b..b7e456e1 100644 > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua > @@ -1,13 +1,18 @@ > +local utils = require "utils" > + > -- Memprof is implemented for x86 and x64 architectures only. > -require("utils").skipcond( > +utils.skipcond( > jit.arch ~= "x86" and jit.arch ~= "x64", > jit.arch.." architecture is NIY for memprof" > ) > > +-- Disabled on *BSD due to #4819. > +utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819') > + We should skip only JIT-related tests here, but not all memprof tests. > local tap = require("tap") > > -local function check_alloc_report(alloc, line, function_line, nevents) > - assert(form_source_line(function_line) == alloc[line].name) > - assert(alloc[line].num == nevents, ("got=%d, expected=%d"):format( > - alloc[line].num, > +local function nevents_eq(event, nevents) > + assert(event.num == nevents, ("got=%d, expected=%d"):format( > + event.num, > + nevents > + )) > +end > + > +local function nevents_gr(event, nevents) > + assert(event.num > nevents, ("got=%d, expected>%d"):format( > + event.num, > nevents > )) > +end > + > +local function check_alloc_report(alloc, traceno, line, function_line, Nit: I suggest to add new fields (traceno, for example) not to the middle of arguments list, but to the end. Feel free to ignore. > + nevents, cmp_events) > + assert(alloc[line], ("no event on line %d"):format(line)) > + local event = alloc[line] > + local expected_name > + if traceno ~= 0 then > + expected_name = string.format("TRACE [%d] ", traceno).. > + form_source_line(function_line) > + else > + expected_name = form_source_line(function_line) > + end > + assert(expected_name == event.name, ("got='%s', expected='%s'"):format( > + event.name, > + expected_name > + )) > + cmp_events(event, nevents) > return true > end > > @@ -107,20 +165,7 @@ test:ok(res == nil and err:match("profiler is not running")) > @@ -166,5 +211,23 @@ local co = coroutine.create(f) > coroutine.resume(co) > misc.memprof.stop() > > +-- Test profiler with enabled JIT. > jit.on() > + > +-- Pregenerate traces to get expected output: Nit: "to fill symtab entry" is more verbose to me. Feel free to ignore. > +default_payload() > + > +symbols, events = generate_parsed_output(default_payload) > + > +alloc = fill_ev_type(events, symbols, "alloc") > + > +-- We expect, that loop will be compiled into a trace. > +-- Depends on system, there will be 99 or 100 allocations Why? Provide an example, please. Maybe we should to change our payload to lock in the behaviour. > +-- from trace, so expect greater than 98. > +test:ok(check_alloc_report(alloc, 1, 35, 30, 98, nevents_gr)) > +-- See same checks with jit.off(). > +test:ok(check_alloc_report(alloc, 0, 32, 30, 2, nevents_eq)) > +-- Collect all previous allocated objects. > +test:ok(free.INTERNAL.num == 102) > + > os.exit(test:check() and 0 or 1) > diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua > index 3ed1dd13..8151ecf8 100644 > --- a/tools/utils/symtab.lua > +++ b/tools/utils/symtab.lua > @@ -10,11 +10,12 @@ local band = bit.band > +local function parse_sym_trace(reader, symtab) > + local traceno = reader:read_uleb128() > + local sym_addr = reader:read_uleb128() > + local sym_line = reader:read_uleb128() > + > + symtab.trace[traceno] = { Assume the following: 1) We get output from 100 traces. 2) Make jit.flush(). 3) We get output from other 100 traces. So all output (from new and old traces) will be reported as output for old traces. So we need at least report trace address as well (yes, collisions may happen). Also, we should report addresses of those functions, to avoid collisions after several jit.flush() calls. It should be removed after symtab enriching [1] will be implemented. Thoughts? > + addr = sym_addr, > + line = sym_line, > + } > +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 +89,29 @@ 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[addr].source, loc.line) > end > + return string_format("CFUNC %#x", addr) > +end > > - if symtab[addr] then > - return string_format("%s:%d", symtab[addr].source, loc.line) > +function M.demangle(symtab, loc) > + local traceno = loc.traceno > + > + if traceno ~= 0 then Nit: we can move this logic (trace related) to the separate function. > + if symtab.trace[traceno] then > + local sym = demangle_lfunc(symtab, symtab.trace[traceno]) > + return string_format("TRACE [%d] ", traceno)..sym Nit: May be it is more userfriendly to add "started at" to avoid misunderstanding that all allocation of this trace are on `sym` line. OTOH, idiomatically this should be done not by demangler, but by humanizer module. Thoughts? Nit: Also, IINM there is no way, when sym is something except the value returned from elseif branch, is it? I suppose, that we can move this string_format to the separate function (`format_lua_sorce_line()`) and call it from here and the elseif branch. > + end > + return string_format("TRACE [%d]", traceno) > end > > - return string_format("CFUNC %#x", addr) > + return demangle_lfunc(symtab, loc) > end > > return M > -- > 2.32.0 > [1]: https://github.com/tarantool/tarantool/issues/5815 -- Best regards, Sergey Kaplun