From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option Date: Tue, 13 Apr 2021 10:43:42 +0300 [thread overview] Message-ID: <20210413074342.GW29703@tarantool.org> (raw) In-Reply-To: <20210331172948.10660-1-skaplun@tarantool.org> Sergey, Thanks for the patch! Now we even have a tests, neat! Besides all comments below, I have a general one regarding the maintaining the tools directory. Essentially it occurs to be totally your domain, and I can't fluently read this code anymore. It definitely relates to the fact I look here once in a three months, but it only confirm my concerns: I'm afraid the code will be hardly maintainable in a year or two. Little comments source-wide in tools directory. I literally read this code with a notebook to write down all structures layout, implicit arguments, etc. We were in a hurry in the previous release. In the current release we were focused on the tests. We have lots of work to be made for memprof enhancement in the next release prior to announcing it as a stable. Then let's polish it in scope of these enhancements. Could you please file a separate issue for refactoring the tools code a bit? On 31.03.21, Sergey Kaplun wrote: > This patch indtroduces new memprof parser module <process.lua> to > post-process memory events. > > Memprof parser now adds postamble with the source lines of Lua chunks Never heard such a word: "postamble". I believe you mean epilogue by that. You also can simply use "report" here. > (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 > --- > Changes in v2: > * introduce new memprof's <process.lua> module to post-process parsed > events > * add tests > > ChangeLog entry (and postamble too Tarantool bump commit message): Feel free to add this into the patch for Tarantool repo. > > =================================================================== > ##feature/luajit > > * Now memory profiler parser reports heap difference occurring during > the measurement interval. New memory profiler's option `--leak-only` > to show only heap difference is introduced. New built-in module > `memprof.process` is introduced to perform memory events > post-processing and aggregation. Now to launch memory profiler > via Tarantool user should use the following command: > `tarantool -e 'require("memprof")(arg)' - --leak-only /tmp/memprof.bin` > =================================================================== > > Branch with tests and added the corresponding built-in: > * https://github.com/tarantool/tarantool/tree/skaplun/gh-5812-memprof-memleaks-option > LuaJIT branch: > * https://github.com/tarantool/luajit/tree/skaplun/gh-5812-memprof-memleaks-option > Issue: https://github.com/tarantool/tarantool/issues/5812 > > .../misclib-memprof-lapi.test.lua | 21 +++++-- > tools/memprof.lua | 33 ++++++----- > tools/memprof/humanize.lua | 43 +++++++++++++- > tools/memprof/parse.lua | 20 +++++-- > tools/memprof/process.lua | 59 +++++++++++++++++++ > 5 files changed, 151 insertions(+), 25 deletions(-) > create mode 100644 tools/memprof/process.lua > > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua > index cb63e1b8..9affc2fe 100644 > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua <snipped> > @@ -120,13 +125,21 @@ local free = fill_ev_type(events, symbols, "free") > -- the number of allocations. > -- 1 event - alocation of table by itself + 1 allocation > -- of array part as far it is bigger than LJ_MAX_COLOSIZE (16). > -test:ok(check_alloc_report(alloc, 20, 18, 2)) > +test:ok(check_alloc_report(alloc, 21, 19, 2)) > -- 100 strings allocations. > -test:ok(check_alloc_report(alloc, 25, 18, 100)) > +test:ok(check_alloc_report(alloc, 26, 19, 100)) Side note: I guess we adjusted these tests almost for each change related either to memprof tests or memprof per se. I guess we need to refactor this later to improve the further maintenance of this spot. > > -- Collect all previous allocated objects. > test:ok(free.INTERNAL.num == 102) > Minor: It's worth to also mention the issue these tests are related to. > +local heap_diff = process.form_heap_diff(events, symbols) > +local tab_alloc_source = heap_diff[form_source_line(21)] > +local str_alloc_source = heap_diff[form_source_line(26)] Minor: Why did you use _source suffix here? I believe it should be <obj>_alloc_stats, shouldn't it? > +test:ok(tab_alloc_source.cnt_alloc == tab_alloc_source.cnt_free) > +test:ok(tab_alloc_source.size_diff == 0) > +test:ok(str_alloc_source.cnt_alloc == str_alloc_source.cnt_free) > +test:ok(str_alloc_source.size_diff == 0) > + > -- Test for https://github.com/tarantool/tarantool/issues/5842. > -- We are not interested in this report. > misc.memprof.start("/dev/null") > diff --git a/tools/memprof.lua b/tools/memprof.lua > index 9f962085..c6c5f587 100644 > --- a/tools/memprof.lua > +++ b/tools/memprof.lua <snipped> > @@ -33,10 +34,16 @@ 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 > > +local leak_only = false > +opt_map["leak-only"] = function() > + leak_only = true > +end > + Side note: I remember we've already discussed that it's better to collect kinda "cfg" or "context" object. Mind this for the further refactoring, please. > -- Print error and exit with error status. > local function opterror(...) > stderr:write("luajit-parse-memprof.lua: ERROR: ", ...) > @@ -94,26 +101,22 @@ 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") > - > + if not leak_only then > + view.profile_info(events, symbols) > + end > + local heap_diff = process.form_heap_diff(events, symbols) > + view.leak_only(heap_diff) I see not a word in issue regarding this change. So you show leaks also when nobody asked you. I personally don't like such approach, but I have no idea what Mons asked you to do. > os.exit(0) > end > > +local function dump_wrapped(...) > + return dump(parseargs(...)) > +end Please, leave a comment regarding this change. > + > -- 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 > diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua > index 2d5814c6..6afd3ff1 100644 > --- a/tools/memprof/humanize.lua > +++ b/tools/memprof/humanize.lua <snipped> > @@ -42,4 +42,43 @@ function M.render(events, symbols) > end > end > > +function M.profile_info(events, symbols) > + print("ALLOCATIONS") Why did you silently change <stdout:write> to <print> here? > + 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(heap_diff) Minor: it's better to name this "leak_info" to fit "profile_info" IMHO. Feel free to ignore. > + local rest_heap = {} OK, here we go again about the naming... You use <heap_diff> that stands for "the heap difference" (right?) and <rest_heap> that means "the rest in the heap" (AFAIU) and "heap" is used as both prefix and suffix. Looks like this code is written by both Jekyll and Hyde. I can provide no strict naming convention, but I can appeal on the common sense. For example, <cnt_alloc> stands for "count allocations" and count is a verb here. Names with verbs in it are likely used for "actions" (i.e. functions). At the same time, <alloc_cnt> stands for "allocations count" where count is a noun and represent an "entity" (i.e. object). Talking about <size_diff> that I read as "size of difference", it's also better to reverse this name to <diff_size> that is read as "difference size". Honestly, neither of them represents the entity better than <diff_bytes> does. Another example are <hold_bytes> and <size_diff>, using units (i.e. "bytes" and "size") via a full word, but "count" contraction "cnt" is used for <cnt_alloc> and <cnt_free>. Please, deal with your Hyde inside and fix the naming changeset-wide. > + for line, info in pairs(heap_diff) do > + -- Report "INTERNAL" events inconsistencies for profiling > + -- with enabled jit. > + if info.size_diff > 0 then > + table.insert(rest_heap, {line = line, hold_bytes = info.size_diff}) > + 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_diff[h.line].cnt_alloc, > + heap_diff[h.line].cnt_free > + )) > + end > + print("") > +end > + > return M > diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua > index 6dae22d5..df10a45f 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] Side note: Well, this is hardly maintainable. Some structures use numeric indices, others -- string keys. <event.primary> represents the list of "heap chunks" (and these are no events AFAIU), but its relations are mentioned nowhere. Not a single word regarding the structures layout. This spot definitely need to be refactored in the next release. > + if not e.primary[heap_chunk[2]] then > + 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 > <snipped> > diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua > new file mode 100644 > index 00000000..94be187e > --- /dev/null > +++ b/tools/memprof/process.lua > @@ -0,0 +1,59 @@ > +-- LuaJIT's memory profile post-processing module. > + > +local M = {} > + > +local symtab = require "utils.symtab" > + > +function M.form_heap_diff(events, symbols) > + -- Auto resurrects source event lines for counting/reporting. > + local heap = setmetatable({}, {__index = function(t, line) > + t[line] = { I'd rather use <rawget> and <rawset> here. Yes, there is no __newindex metamethod now, but using <raw*> methods looks to be foolproof. > + size_diff = 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) > + > + if (event.alloc > 0) then > + heap[ev_line].size_diff = heap[ev_line].size_diff + 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. And what is the difference in alloc events except they have no <primary> list linking the memory manipulations? > + -- 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. > + local function process_non_alloc_events(events_by_type) Why do you need to define the function right here? To omit <heap> and <symbols> parameters? For what? > + 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) > + > + if (heap_chunk.alloced > 0) then > + heap[ev_line].size_diff = heap[ev_line].size_diff + heap_chunk.alloced > + heap[ev_line].cnt_alloc = heap[ev_line].cnt_alloc + heap_chunk.cnt > + end > + > + if (heap_chunk.freed > 0) then > + heap[ev_line].size_diff = heap[ev_line].size_diff - heap_chunk.freed > + 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) > + return heap Again about naming: you already use <heap> in memprof/parse.lua, but AFAIU it provides a different structure. Furthermore, you return <heap> here, but exactly this result is stored in <heap_diff> later. After all, is this <heap> or <heap_diff>? > +end > + > +return M > -- > 2.31.0 > -- Best regards, IM
next prev parent reply other threads:[~2021-04-13 7:43 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-31 17:29 Sergey Kaplun via Tarantool-patches 2021-04-08 12:49 ` Sergey Ostanevich via Tarantool-patches 2021-04-14 11:32 ` Sergey Kaplun via Tarantool-patches 2021-04-13 7:43 ` Igor Munkin via Tarantool-patches [this message] 2021-04-14 11:31 ` Sergey Kaplun via Tarantool-patches 2021-04-14 13:12 ` Igor Munkin via Tarantool-patches 2021-04-14 14:34 ` Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210413074342.GW29703@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox