Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.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