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

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

[Sergey Kaplun via Tarantool-patches]

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

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?

The example of output. Assuming we have file <format_concat.lua>:
|  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
<tools/memprof/parse.lua> 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()
+  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")
-
+  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))
     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
+  -- 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)
+    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
+          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
+      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

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

[Sergey Ostanevich via Tarantool-patches]

Hi! 
Thanks for the patch, some comments below.

Regards,
Sergos


> On 25 Jan 2021, at 22:35, Sergey Kaplun <skaplun@tarantool.org> 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
> 
> 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]

> 
> The example of output. Assuming we have file <format_concat.lua>:
> |  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
> <tools/memprof/parse.lua> 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?

> +  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.

> +  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?

>     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
> +  -- 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?

> +  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.

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

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

[Sergey Kaplun via 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 <skaplun@tarantool.org> 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 <format_concat.lua>:
> > |  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
> > <tools/memprof/parse.lua> 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

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

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! This is definitely not "view" in terms of memprof.
This is a routine that post-process the parsed and grouped data. There
was no such stage before: the binary profile is parsed into events and
then events are rendered in a human-readable format. If you introduced
the new output format (e.g. JSON, YAML, etc), then it would be a "view",
since we don't need to test how Lua tables are converted into JSON
objects. But in this patch, you post-process the parsed events into
another structure based on the user's request (i.e. show only leaks).
Hence, this post-processing *need to be tested* at least as a so-called
"black box". You provide no tests for you patch at all, so if something
is broken we'll know it only from the customer's feedback.

I propose to reconsider the way you introduced this feature and provide
some tests for it (the latter is the main problem).

-- 
Best regards,
IM

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

[Sergey Ostanevich via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 18235 bytes --]

Updates are LGTM.

Although, I support Igor on his request to add some tests for alloc-free pairs detection.

Sergos
 

> On 17 Mar 2021, at 18:03, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> 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 <skaplun@tarantool.org> 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 <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 <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 <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 <format_concat.lua>:
>>> |  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
>>> <tools/memprof/parse.lua> 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 <https://github.com/tarantool/tarantool/issues/5812>
> 
> -- 
> Best regards,
> Sergey Kaplun


[-- Attachment #2: Type: text/html, Size: 160182 bytes --]