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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi!

Thanks for the review!

On 08.04.21, Sergey Ostanevich wrote:
> Hi!
> 
> Just couple of nits, LGTM.
> 
> Sergos
> 
> > On 31 Mar 2021, at 20:29, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > This patch indtroduces new memprof parser module <process.lua> to
> 	     introduces

Fixed.

> > post-process memory events.
> > 
> > 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
> > ---
> > 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):
>                                  ^^^^^^^ I failed to parse, typo?

Yep :).

> > 
> > ===================================================================
> > ##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
>     shows

Fixed on branch.

> >  `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
> > @@ -1,7 +1,7 @@
> > local tap = require("tap")
> > 
> > local test = tap.test("misc-memprof-lapi")
> > -test:plan(9)
> > +test:plan(13)
> > 
> > jit.off()
> > jit.flush()
> > @@ -10,6 +10,7 @@ local table_new = require "table.new"
> > 
> > local bufread = require "utils.bufread"
> > local memprof = require "memprof.parse"
> > +local process = require "memprof.process"
> > local symtab = require "utils.symtab"
> > 
> > local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.memprofdata.tmp.bin")
> > @@ -66,8 +67,12 @@ local function fill_ev_type(events, symbols, event_type)
> >   return ev_type
> > end
> > 
> > +local function form_source_line(line)
> > +  return string.format("@%s:%d", arg[0], line)
> > +end
> > +
> > local function check_alloc_report(alloc, line, function_line, nevents)
> > -  assert(string.format("@%s:%d", arg[0], function_line) == alloc[line].name)
> > +  assert(form_source_line(function_line) == alloc[line].name)
> >   assert(alloc[line].num == nevents, ("got=%d, expected=%d"):format(
> >     alloc[line].num,
> >     nevents
> > @@ -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
> 	       allocation

Nice catch! Unfortunately, it is not relating to this patch, so I left it
intact.

> > -- 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))
> > 
> > -- Collect all previous allocated objects.
> > test:ok(free.INTERNAL.num == 102)
> > 
> > +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)]
> > +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
> > @@ -12,6 +12,7 @@
> > 
> > local bufread = require "utils.bufread"
> > local memprof = require "memprof.parse"
> > +local process = require "memprof.process"
> > local symtab = require "utils.symtab"
> > local view = require "memprof.humanize"
> > 
> > @@ -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
> > +
> > -- 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)
> 
> The name of the function is confusing: you dump whole data if _not_ only
> leaks, and then without alternative the leaks data. It sounds as ‘always’
> but I propose to name it just ‘leaks’. Then the logic will be ‘dump all’
> or ‘leaks’ only.

Changed to `leak_info()`, according to Igor's review; see the iterative
patch below:

===================================================================
diff --git a/tools/memprof.lua b/tools/memprof.lua
index c6c5f587..39505020 100644
--- a/tools/memprof.lua
+++ b/tools/memprof.lua
@@ -105,7 +105,7 @@ local function dump(inputfile)
     view.profile_info(events, symbols)
   end
   local heap_diff = process.form_heap_diff(events, symbols)
-  view.leak_only(heap_diff)
+  view.leak_info(heap_diff)
   os.exit(0)
 end
 
diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
index 6afd3ff1..3812c6c4 100644
--- a/tools/memprof/humanize.lua
+++ b/tools/memprof/humanize.lua
@@ -56,7 +56,7 @@ function M.profile_info(events, symbols)
   print("")
 end
 
-function M.leak_only(heap_diff)
+function M.leak_info(heap_diff)
   local rest_heap = {}
   for line, info in pairs(heap_diff) do
     -- Report "INTERNAL" events inconsistencies for profiling
===================================================================

> 

<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 @@

<snipped>

> > +  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
>                                                a
> > +      -- that references table with rewrited memory
>                           the          rewritten, although I’d use a different verb
>                                        and put at the end: ‘memory changed’?

Fixed. See the iterative patch below.

===================================================================
diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua
index 64acfd15..34747dec 100644
--- a/tools/memprof/process.lua
+++ b/tools/memprof/process.lua
@@ -33,8 +33,8 @@ function M.form_heap_delta(events, symbols)
   -- no collisions between different object addresses.
   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
+      -- Realloc and free events always have key named "primary"
+      -- 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)
===================================================================

> > +      -- (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
> > +end
> > +
> > +return M
> > -- 
> > 2.31.0
> > 
> 

-- 
Best regards,
Sergey Kaplun