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 3FFA06EC56; Wed, 17 Mar 2021 18:04:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3FFA06EC56 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1615993456; bh=azcWjByI7bde6BFzBYqoW7aQlie6jUbkNsFwex0qKI4=; 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=I9pqobWxOA9ryeaCx0i6VR/8z7ufbQ73ac/4vo/fZCmW49GWmp+PXb7zhHIyUhZav ro6OrpEulWaGoM2bWu8NsSE/oGu4y3vbyoLdq8Id8Rn5NEe5TNnbv8KGvqWKVVfqRt C0K8LWM0UPoN81WyVEewNdK4RYMgGiY1mF9x6kEQ= Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 679266EC56 for ; Wed, 17 Mar 2021 18:04:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 679266EC56 Received: by smtp42.i.mail.ru with esmtpa (envelope-from ) id 1lMXiY-0001yv-D6; Wed, 17 Mar 2021 18:04:14 +0300 Date: Wed, 17 Mar 2021 18:03:23 +0300 To: Sergey Ostanevich Message-ID: <20210317150323.GB28016@root> References: <20210125193544.18050-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD96485A7A9FC1318934FEBC8F0BC2797F6B85C225F6ECFBC4F182A05F5380850401DFB899B098AABB1CCB0E2D449F4EF0A74250A2D976D65DA917DF5FFD401D31C X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE765EC468FF5F3030AEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A03F291446A1FA038638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95CF89CA98302ED496FC8F8621BA0BE4323A59531EF0C640050A471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23DF004C9065253843057739F23D657EF2B13377AFFFEAFD26923F8577A6DFFEA7C4DB04CA562B6C7F893EC92FD9297F6715571747095F342E857739F23D657EF2BD5E8D9A59859A8B6F6B3B5B76D0D00FF089D37D7C0E48F6C5571747095F342E857739F23D657EF2B6825BDBE14D8E7028C9DFF55498CEFB0BD9CCCA9EDD067B1EDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5DE1E312E6BBC6BF27EC4F9231E2466F267D28B3D7D4BE068D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75F04B387B5D7535DE410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34EA882B598A209811E47D86D8C322075EB0FDC7085DDD5084C8466414BBEB0DA3170ECB36FA1B626F1D7E09C32AA3244C3E6566DF3DF3F100C77A0E37CB2E3420CE0B41342B755BCDFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojwV/GgY7Z4vV5IpT2utRVzA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB4D5DBF82D9BFCC9D17B56306623EF0D31CA051C13401809B8F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v1] tools: introduce --leak-only memprof parser option 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! Thanks for the review! On 29.01.21, Sergey Ostanevich wrote: > Hi! > Thanks for the patch, some comments below. > > Regards, > Sergos > > > > On 25 Jan 2021, at 22:35, Sergey Kaplun wrote: > > > > This patch adds a new --leak-only memory profiler parser option. > > When the parser runs with that option, it will report only lines > > (or "INTERNAL") that allocate and do not free some amount of bytes. > > The parser also reports the number of allocation and deallocation > > events related to each line. > > --- > > > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-memprof-memleaks-option > > Testing branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-memprof-memleaks-option I removed the old branches and create issue[1]-related: Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5812-memprof-memleaks-option Testing branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5812-memprof-memleaks-option > > > > Side note: I only update commit message thats why CI from the "old" > > commit. > > CI: https://gitlab.com/tarantool/tarantool/-/pipelines/246541599 > > > > This patch is a result of offline discussion of memory profiler > > and its usage and feedback from Mons. > > > > I don't know - is it reasonable to reference [1] issue here or create a > > new one? > > I think it’s ok to mention [1] Updated commit message to the following: | tools: introduce --leak-only memprof parser option | | Memprof parser now adds postamble with the source lines of Lua chunks | (or "INTERNAL") that allocate and do not free some amount of bytes, when | profiler finishes. The parser also reports the number of allocation and | deallocation events related to each line. | | Also, this patch adds a new --leak-only memory profiler parser option. | When the parser runs with that option, it reports only leak | information. | | Resolves tarantool/tarantool#5812 Also, I've added the following patch to be able to run memprof from Tarantool with leak-only option. =================================================================== diff --git a/tools/memprof.lua b/tools/memprof.lua index 85dadd4..d2231ee 100644 --- a/tools/memprof.lua +++ b/tools/memprof.lua @@ -107,10 +107,14 @@ local function dump(inputfile) os.exit(0) end +local function dump_wrapped(...) + return dump(parseargs(...)) +end + -- FIXME: this script should be application-independent. local args = {...} if #args == 1 and args[1] == "memprof" then - return dump + return dump_wrapped else - dump(parseargs(args)) + dump_wrapped(args) end =================================================================== Now memprof parser can be called from Tarantool with --leak-only option like the following: | $ src/tarantool -e 'local memprof = require("memprof")(arg)' - --leak-only /tmp/memprof.bin Thoughts? > > > > > The example of output. Assuming we have file : > > | 1 jit.off() -- more verbose reports > > | 2 > > | 3 local function concat(a) > > | 4 local nstr = a.."a" > > | 5 return nstr > > | 6 end > > | 7 > > | 8 local function format(a) > > | 9 local nstr = string.format("%sa", a) > > | 10 return nstr > > | 11 end > > | 12 > > | 13 collectgarbage() -- cleanup > > | 14 > > | 15 local binfile = "/tmp/memprof_"..(arg[0]):match("/([^/]*).lua")..".bin" > > | 16 > > | 17 local st, err = misc.memprof.start(binfile) > > | 18 assert(st, err) > > | 19 > > | 20 for i = 1, 10000 do > > | 21 local f = format(i) > > | 22 local c = concat(i) > > | 23 end > > | 24 > > | 25 local st, err = misc.memprof.stop() > > | 26 assert(st, err) > > | 27 > > | 28 os.exit() > > > > Parser's output without option: > > | ALLOCATIONS > > | @/home/burii/reports/demo_memprof/format_concat.lua:8, line 9: 19998 624322 0 > > | > > | REALLOCATIONS > > | > > | DEALLOCATIONS > > | INTERNAL: 283 0 5602 > > | Overrides: > > | @/home/burii/reports/demo_memprof/format_concat.lua:8, line 9 > > | > > | @/home/burii/reports/demo_memprof/format_concat.lua:8, line 9: 2 0 98304 > > | Overrides: > > | @/home/burii/reports/demo_memprof/format_concat.lua:8, line 9 > > > > And with: > > > > | HEAP SUMMARY: > > | @/home/burii/reports/demo_memprof/format_concat.lua:8, line 9 holds 553214 bytes 19998 allocs, 283 frees > > > > Side note: The attentive reader will notice that we have 283+2 frees in > > the first case and 283 in the second. > > This is because --leak-only considers deallocation/reallocation events > > for which we know the source address. The default report aggregates > > these all deallocations into one. See `link_to_previous()` in > > for details. > > > > tools/memprof.lua | 23 +++++----- > > tools/memprof/humanize.lua | 91 +++++++++++++++++++++++++++++++++++++- > > tools/memprof/parse.lua | 20 +++++++-- > > 3 files changed, 115 insertions(+), 19 deletions(-) > > > > diff --git a/tools/memprof.lua b/tools/memprof.lua > > index 9f96208..5d09fbd 100644 > > --- a/tools/memprof.lua > > +++ b/tools/memprof.lua > > @@ -37,6 +37,11 @@ Supported options are: > > os.exit(0) > > end > > > > +local leak_only = false > > +opt_map["leak-only"] = function() > > Should you add this into the SYNOPSIS part? Yes, thanks, see the iterative patch below: =================================================================== diff --git a/tools/memprof.lua b/tools/memprof.lua index 5d09fbd..e1af99f 100644 --- a/tools/memprof.lua +++ b/tools/memprof.lua @@ -33,6 +33,7 @@ luajit-parse-memprof [options] memprof.bin Supported options are: --help Show this help and exit + --leak-only Report only leaks information ]] os.exit(0) end =================================================================== > > > + leak_only = true > > +end > > + > > -- Print error and exit with error status. > > local function opterror(...) > > stderr:write("luajit-parse-memprof.lua: ERROR: ", ...) > > @@ -94,19 +99,11 @@ local function dump(inputfile) > > local reader = bufread.new(inputfile) > > local symbols = symtab.parse(reader) > > local events = memprof.parse(reader, symbols) > > - > > - stdout:write("ALLOCATIONS", "\n") > > - view.render(events.alloc, symbols) > > - stdout:write("\n") > > - > > - stdout:write("REALLOCATIONS", "\n") > > - view.render(events.realloc, symbols) > > - stdout:write("\n") > > - > > - stdout:write("DEALLOCATIONS", "\n") > > - view.render(events.free, symbols) > > - stdout:write("\n") > > - > > Should the HEAP SUMMARY appear in the end of the ‘all’ report? > Otherwise the ‘_only’ in the option name is mesleading: show part of > the report, while it shows completely different one. Same for the > code below. Agree, fixed. Also, I renamed `all()` function name to the more verbose one: =================================================================== diff --git a/tools/memprof.lua b/tools/memprof.lua index e1af99f..85dadd4 100644 --- a/tools/memprof.lua +++ b/tools/memprof.lua @@ -100,11 +100,10 @@ local function dump(inputfile) local reader = bufread.new(inputfile) local symbols = symtab.parse(reader) local events = memprof.parse(reader, symbols) - if leak_only then - view.leak_only(events, symbols) - else - view.all(events, symbols) + if not leak_only then + view.profile_info(events, symbols) end + view.leak_only(events, symbols) os.exit(0) end diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua index bad0597..9e1bdf2 100644 --- a/tools/memprof/humanize.lua +++ b/tools/memprof/humanize.lua @@ -42,7 +42,7 @@ function M.render(events, symbols) end end -function M.all(events, symbols) +function M.profile_info(events, symbols) print("ALLOCATIONS") M.render(events.alloc, symbols) print("") =================================================================== > > > + if leak_only then > > + view.leak_only(events, symbols) > > + else > > + view.all(events, symbols) > > + end > > os.exit(0) > > end > > > > diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua > > index 109a39d..bad0597 100644 > > --- a/tools/memprof/humanize.lua > > +++ b/tools/memprof/humanize.lua > > @@ -28,8 +28,8 @@ function M.render(events, symbols) > > )) > > > > local prim_loc = {} > > - for _, loc in pairs(event.primary) do > > - table.insert(prim_loc, symtab.demangle(symbols, loc)) > > + for _, heap_chunk in pairs(event.primary) do > > + table.insert(prim_loc, symtab.demangle(symbols, heap_chunk.loc)) > > The order of allocations is changed, was it incorrect in some way? The `event.primary` array is modified and contains now not only location line as a string, but a table with the line location in Lua chunk, plus info about amount of allocated and freed bytes. > > > end > > if #prim_loc ~= 0 then > > table.sort(prim_loc) > > @@ -42,4 +42,91 @@ function M.render(events, symbols) > > end > > end > > > > +function M.all(events, symbols) > > + print("ALLOCATIONS") > > + M.render(events.alloc, symbols) > > + print("") > > + > > + print("REALLOCATIONS") > > + M.render(events.realloc, symbols) > > + print("") > > + > > + print("DEALLOCATIONS") > > + M.render(events.free, symbols) > > + print("") > > +end > > + > > +function M.leak_only(events, symbols) > > + -- Auto resurrects source event lines for counting/reporting. > > + local heap = setmetatable({}, {__index = function(t, line) > > + t[line] = { > > + size = 0, > > + cnt_alloc = 0, > > + cnt_free = 0, > > + } > > + return t[line] > > + end}) > > + > > + for _, event in pairs(events.alloc) do > > + if event.loc then > > + local ev_line = symtab.demangle(symbols, event.loc) > > + > > + heap[ev_line].size = heap[ev_line].size + event.alloc > > + if (event.alloc > 0) then > > + heap[ev_line].cnt_alloc = heap[ev_line].cnt_alloc + event.num > > + end > > + end > > + end > > + > > + -- Realloc and free events are pretty the same. > > + -- We don't interesting in aggregated alloc/free sizes for > aren’t interested Fixed, thanks! See the iterative patch below. > > + -- the event, but only for new and old size values inside > > + -- alloc-realloc-free chain. Assuming that we have > > + -- no collisions between different object addresses. > > I believe the address is source of the chunk {_|de|re}allocation, is there any > different one? I mean that, if some line allocated a few bytes in 0x4eadbeaf address, freed and the other one line allocated data at the same address later, it will be aggregated together. > > > + local function process_non_alloc_events(events_by_type) > > + for _, event in pairs(events_by_type) do > > + -- Realloc and free events always have "primary" key > > + -- that references table with rewrited memory > > + -- (may be empty). > > + for _, heap_chunk in pairs(event.primary) do > > + local ev_line = symtab.demangle(symbols, heap_chunk.loc) > > + > > + heap[ev_line].size = heap[ev_line].size + heap_chunk.alloced > > + if (heap_chunk.alloced > 0) then > > Can it be negative? If not - the above line can be put under this if also. No, it can't, see the iterative patch below: =================================================================== diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua index 9e1bdf2..b8dbfbf 100644 --- a/tools/memprof/humanize.lua +++ b/tools/memprof/humanize.lua @@ -71,15 +71,15 @@ function M.leak_only(events, symbols) if event.loc then local ev_line = symtab.demangle(symbols, event.loc) - heap[ev_line].size = heap[ev_line].size + event.alloc if (event.alloc > 0) then + heap[ev_line].size = heap[ev_line].size + event.alloc heap[ev_line].cnt_alloc = heap[ev_line].cnt_alloc + event.num end end end -- Realloc and free events are pretty the same. - -- We don't interesting in aggregated alloc/free sizes for + -- We aren't interested in aggregated alloc/free sizes for -- the event, but only for new and old size values inside -- alloc-realloc-free chain. Assuming that we have -- no collisions between different object addresses. @@ -91,13 +91,13 @@ function M.leak_only(events, symbols) for _, heap_chunk in pairs(event.primary) do local ev_line = symtab.demangle(symbols, heap_chunk.loc) - heap[ev_line].size = heap[ev_line].size + heap_chunk.alloced if (heap_chunk.alloced > 0) then + heap[ev_line].size = heap[ev_line].size + heap_chunk.alloced heap[ev_line].cnt_alloc = heap[ev_line].cnt_alloc + heap_chunk.cnt end - heap[ev_line].size = heap[ev_line].size - heap_chunk.freed if (heap_chunk.freed > 0) then + heap[ev_line].size = heap[ev_line].size - heap_chunk.freed heap[ev_line].cnt_free = heap[ev_line].cnt_free + heap_chunk.cnt end end =================================================================== > > > + heap[ev_line].cnt_alloc = heap[ev_line].cnt_alloc + heap_chunk.cnt > > + end > > + > > + heap[ev_line].size = heap[ev_line].size - heap_chunk.freed > > + if (heap_chunk.freed > 0) then > > + heap[ev_line].cnt_free = heap[ev_line].cnt_free + heap_chunk.cnt > > + end > > + end > > + end > > + end > > + process_non_alloc_events(events.realloc) > > + process_non_alloc_events(events.free) > > + > > + local rest_heap = {} > > + for line, info in pairs(heap) do > > + -- Report "INTERNAL" events inconsistencies for profiling > > + -- with enabled jit. > > + if info.size > 0 then > > + table.insert(rest_heap, {line = line, hold_bytes = info.size}) > > + end > > + end > > + > > + table.sort(rest_heap, function(h1, h2) > > + return h1.hold_bytes > h2.hold_bytes > > + end) > > + > > + print("HEAP SUMMARY:") > > + for _, h in pairs(rest_heap) do > > + print(string.format( > > + "%s holds %d bytes %d allocs, %d frees", > > + h.line, h.hold_bytes, heap[h.line].cnt_alloc, heap[h.line].cnt_free > > + )) > > + end > > + print("") > > +end > > + > > return M > > diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua > > index 6dae22d..374686c 100644 > > --- a/tools/memprof/parse.lua > > +++ b/tools/memprof/parse.lua > > @@ -39,11 +39,23 @@ local function new_event(loc) > > } > > end > > > > -local function link_to_previous(heap_chunk, e) > > +local function link_to_previous(heap_chunk, e, nsize) > > -- Memory at this chunk was allocated before we start tracking. > > if heap_chunk then > > -- Save Lua code location (line) by address (id). > > - e.primary[heap_chunk[2]] = heap_chunk[3] > > + if e.primary[heap_chunk[2]] == nil then I don't like this line now, so I've updated it like the following: =================================================================== diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua index 374686c..df10a45 100644 --- a/tools/memprof/parse.lua +++ b/tools/memprof/parse.lua @@ -43,7 +43,7 @@ local function link_to_previous(heap_chunk, e, nsize) -- Memory at this chunk was allocated before we start tracking. if heap_chunk then -- Save Lua code location (line) by address (id). - if e.primary[heap_chunk[2]] == nil then + if not e.primary[heap_chunk[2]] then e.primary[heap_chunk[2]] = { loc = heap_chunk[3], alloced = 0, =================================================================== > > + e.primary[heap_chunk[2]] = { > > + loc = heap_chunk[3], > > + alloced = 0, > > + freed = 0, > > + cnt = 0, > > + } > > + end > > + -- Save memory diff heap information. > > + local location_data = e.primary[heap_chunk[2]] > > + location_data.alloced = location_data.alloced + nsize > > + location_data.freed = location_data.freed + heap_chunk[1] > > + location_data.cnt = location_data.cnt + 1 > > end > > end > > > > @@ -97,7 +109,7 @@ local function parse_realloc(reader, asource, events, heap) > > e.free = e.free + osize > > e.alloc = e.alloc + nsize > > > > - link_to_previous(heap[oaddr], e) > > + link_to_previous(heap[oaddr], e, nsize) > > > > heap[oaddr] = nil > > heap[naddr] = {nsize, id, loc} > > @@ -116,7 +128,7 @@ local function parse_free(reader, asource, events, heap) > > e.num = e.num + 1 > > e.free = e.free + osize > > > > - link_to_previous(heap[oaddr], e) > > + link_to_previous(heap[oaddr], e, 0) > > > > heap[oaddr] = nil > > end > > -- > > 2.28.0 > > > > [1]: https://github.com/tarantool/tarantool/issues/5657 > [1]: https://github.com/tarantool/tarantool/issues/5812 -- Best regards, Sergey Kaplun