Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option
@ 2021-03-31 17:29 Sergey Kaplun via Tarantool-patches
  2021-04-08 12:49 ` Sergey Ostanevich via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-03-31 17:29 UTC (permalink / raw)
  To: Sergey Ostanevich, Igor Munkin; +Cc: tarantool-patches

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
(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):

===================================================================
##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
@@ -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
 -- 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)
   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
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
@@ -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,43 @@ function M.render(events, symbols)
   end
 end
 
+function M.profile_info(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(heap_diff)
+  local rest_heap = {}
+  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]
+    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
 
@@ -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
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] = {
+      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.
+  -- 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)
+    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
+end
+
+return M
-- 
2.31.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option
  2021-03-31 17:29 [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option 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
  2021-04-14 14:34 ` Igor Munkin via Tarantool-patches
  2 siblings, 1 reply; 7+ messages in thread
From: Sergey Ostanevich via Tarantool-patches @ 2021-04-08 12:49 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

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
> 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?
> 
> ===================================================================
> ##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
>  `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
> -- 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.

>   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
> 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
> @@ -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,43 @@ function M.render(events, symbols)
>   end
> end
> 
> +function M.profile_info(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(heap_diff)
> +  local rest_heap = {}
> +  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]
> +    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
> 
> @@ -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
> 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] = {
> +      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.
> +  -- 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)
> +    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’?
> +      -- (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
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option
  2021-03-31 17:29 [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option Sergey Kaplun via Tarantool-patches
  2021-04-08 12:49 ` Sergey Ostanevich via Tarantool-patches
@ 2021-04-13  7:43 ` Igor Munkin via Tarantool-patches
  2021-04-14 11:31   ` Sergey Kaplun via Tarantool-patches
  2021-04-14 14:34 ` Igor Munkin via Tarantool-patches
  2 siblings, 1 reply; 7+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-04-13  7:43 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option
  2021-04-13  7:43 ` Igor Munkin via Tarantool-patches
@ 2021-04-14 11:31   ` Sergey Kaplun via Tarantool-patches
  2021-04-14 13:12     ` Igor Munkin via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-04-14 11:31 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

Igor,

Thanks for the review!

On 13.04.21, Igor Munkin wrote:
> 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?

Yes, see [1].

Also I suggest to create a Code style guidelines for LuaJIT (as far as
it is totally different from Tarantool's codestyle) for C and Lua.
Looks like this guidelines is good for self-checking before sending
patch for the review.
Thoughts?

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

It is modelled on preamble, with post- replacing pre-.
Changed to epilogue.

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

I'm a little bit confused here. How should I proceed the patch for the
Tarantool repo? Usually Kirill or somebody just bumping version of a
submodule with list of commits. Should I create the separate patch for
this? Also, IINM, you asked me not to reference the issues in the commit
message for Tarantool's luajit bump to avoid extra mentioning, because
you anyway need to mention the issue during the bumping.

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

Totally agree. I suppose we should have more generic test module
for profiler's report (not only to memprof).

I suppose it should be in the separate issue, not related to memprof
refactoring.

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

Fixed. See the patch near the comment below.

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

Yep, "stats" sounds better, fixed. The iterative patch:

===================================================================
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index 9affc2fe..508834df 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -132,13 +132,15 @@ test:ok(check_alloc_report(alloc, 26, 19, 100))
 -- Collect all previous allocated objects.
 test:ok(free.INTERNAL.num == 102)

+-- Tests for leak-only option.
+-- See also https://github.com/tarantool/tarantool/issues/5812.
 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)
+local tab_alloc_stats = heap_diff[form_source_line(21)]
+local str_alloc_stats = heap_diff[form_source_line(26)]
+test:ok(tab_alloc_stats.cnt_alloc == tab_alloc_stats.cnt_free)
+test:ok(tab_alloc_stats.size_diff == 0)
+test:ok(str_alloc_stats.cnt_alloc == str_alloc_stats.cnt_free)
+test:ok(str_alloc_stats.size_diff == 0)

 -- Test for https://github.com/tarantool/tarantool/issues/5842.
 -- We are not interested in this report.
===================================================================

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

"config" looks more habitually for the utilities. "context" is more
wide-using for the requests or something like them, in my opinion.

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

It was discussed with Sergos in the previous version. Also, AFAIR, Mons
really asked to show this information always.

> 
> >    os.exit(0)
> >  end
> >  
> > +local function dump_wrapped(...)
> > +  return dump(parseargs(...))
> > +end
> 
> Please, leave a comment regarding this change.

Fixed. See the iterative patch below:

===================================================================
diff --git a/tools/memprof.lua b/tools/memprof.lua
index 39505020..9b27fe87 100644
--- a/tools/memprof.lua
+++ b/tools/memprof.lua
@@ -109,6 +109,10 @@ local function dump(inputfile)
   os.exit(0)
 end
 
+-- XXX: When this script is used as a preload module by an
+-- application, it should return one function for correct parsing
+-- of command line flags like --leak-only and dumping profile
+-- info.
 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
> > 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?

This module uses `print()` everywhere. <memprof.lua> uses stderr to
write error messages at start and uses stdout for these messages.
But as you can see `render()` function already uses just `print()`.
So I prefer to use the same way of the output.

Side note: what do you mean by "silently"? How should I comment these
changes?

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

Fixed. 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
===================================================================

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

Fixed. Changed `form_heap_diff()` to `form_heap_delta()` and
`heap_diff` to `dheap` stans for "the heap delta" (delta during
profiling). Also, changed `rest_heap` to `leaks`. The leaks criteria
for now is that delta bytes during profiling is more than 0.
See the iterative patch below:

===================================================================
diff --git a/tools/memprof.lua b/tools/memprof.lua
index 9b27fe87..b3d576c9 100644
--- a/tools/memprof.lua
+++ b/tools/memprof.lua
@@ -104,8 +104,8 @@ local function dump(inputfile)
   if not leak_only then
     view.profile_info(events, symbols)
   end
-  local heap_diff = process.form_heap_diff(events, symbols)
-  view.leak_info(heap_diff)
+  local dheap = process.form_heap_delta(events, symbols)
+  view.leak_info(dheap)
   os.exit(0)
 end
 
diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
index db801800..7771005d 100644
--- a/tools/memprof/humanize.lua
+++ b/tools/memprof/humanize.lua
@@ -56,26 +56,26 @@ function M.profile_info(events, symbols)
   print("")
 end
 
-function M.leak_info(heap_diff)
-  local rest_heap = {}
-  for line, info in pairs(heap_diff) do
+function M.leak_info(dheap)
+  local leaks = {}
+  for line, info in pairs(dheap) do
     -- Report "INTERNAL" events inconsistencies for profiling
     -- with enabled jit.
     if info.dbytes > 0 then
-      table.insert(rest_heap, {line = line, dbytes = info.dbytes})
+      table.insert(leaks, {line = line, dbytes = info.dbytes})
     end
   end
 
-  table.sort(rest_heap, function(h1, h2)
-    return h1.dbytes > h2.dbytes
+  table.sort(leaks, function(l1, l2)
+    return l1.dbytes > l2.dbytes
   end)
 
   print("HEAP SUMMARY:")
-  for _, h in pairs(rest_heap) do
+  for _, l in pairs(leaks) do
     print(string.format(
       "%s holds %d bytes: %d allocs, %d frees",
-      h.line, h.dbytes, heap_diff[h.line].nalloc,
-      heap_diff[h.line].nfree
+      l.line, l.dbytes, dheap[l.line].nalloc,
+      dheap[l.line].nfree
     ))
   end
   print("")
diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua
index 9278b07c..83920be2 100644
--- a/tools/memprof/process.lua
+++ b/tools/memprof/process.lua
@@ -4,9 +4,9 @@ local M = {}
 
 local symtab = require "utils.symtab"
 
-function M.form_heap_diff(events, symbols)
+function M.form_heap_delta(events, symbols)
   -- Auto resurrects source event lines for counting/reporting.
-  local heap = setmetatable({}, {__index = function(t, line)
+  local dheap = setmetatable({}, {__index = function(t, line)
     t[line] = {
       dbytes = 0,
       nalloc = 0,
@@ -20,8 +20,8 @@ function M.form_heap_diff(events, symbols)
       local ev_line = symtab.demangle(symbols, event.loc)
 
       if (event.alloc > 0) then
-        heap[ev_line].dbytes = heap[ev_line].dbytes + event.alloc
-        heap[ev_line].nalloc = heap[ev_line].nalloc + event.num
+        dheap[ev_line].dbytes = dheap[ev_line].dbytes + event.alloc
+        dheap[ev_line].nalloc = dheap[ev_line].nalloc + event.num
       end
     end
   end
@@ -36,24 +36,24 @@ function M.form_heap_diff(events, symbols)
         local ev_line = symtab.demangle(symbols, heap_chunk.loc)
 
         if (heap_chunk.alloced > 0) then
-          heap[ev_line].dbytes = heap[ev_line].dbytes + heap_chunk.alloced
-          heap[ev_line].nalloc = heap[ev_line].nalloc + heap_chunk.cnt
+          dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.alloced
+          dheap[ev_line].nalloc = dheap[ev_line].nalloc + heap_chunk.cnt
         end
 
         if (heap_chunk.freed > 0) then
-          heap[ev_line].dbytes = heap[ev_line].dbytes - heap_chunk.freed
-          heap[ev_line].nfree = heap[ev_line].nfree + heap_chunk.cnt
+          dheap[ev_line].dbytes = dheap[ev_line].dbytes - heap_chunk.freed
+          dheap[ev_line].nfree = dheap[ev_line].nfree + heap_chunk.cnt
         end
       end
     end
   end
   process_non_alloc_events(events.realloc)
   process_non_alloc_events(events.free)
-  return heap
+  return dheap
 end
 
 return M
===================================================================

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

Changed to `nalloc` and `nfree` stands for "number of allocations" and
"number of frees" correspondingly. See the iterative patch below.

===================================================================
diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
index 3812c6c4..d990af67 100644
--- a/tools/memprof/humanize.lua
+++ b/tools/memprof/humanize.lua
@@ -74,8 +74,8 @@ function M.leak_info(heap_diff)
   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
+      h.line, h.hold_bytes, heap_diff[h.line].nalloc,
+      heap_diff[h.line].nfree
     ))
   end
   print("")
diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua
index 94be187e..d3e944c7 100644
--- a/tools/memprof/process.lua
+++ b/tools/memprof/process.lua
@@ -9,8 +9,8 @@ function M.form_heap_diff(events, symbols)
   local heap = setmetatable({}, {__index = function(t, line)
     t[line] = {
       size_diff = 0,
-      cnt_alloc = 0,
-      cnt_free = 0,
+      nalloc = 0,
+      nfree = 0,
     }
     return t[line]
   end})
@@ -21,7 +21,7 @@ function M.form_heap_diff(events, symbols)
 
       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
+        heap[ev_line].nalloc = heap[ev_line].nalloc + event.num
       end
     end
   end
@@ -41,12 +41,12 @@ function M.form_heap_diff(events, symbols)
 
         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
+          heap[ev_line].nalloc = heap[ev_line].nalloc + 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
+          heap[ev_line].nfree = heap[ev_line].nfree + heap_chunk.cnt
         end
       end
     end
===================================================================

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

Changed to `dbytes` stands for "difference in bytes" or "delta bytes".
See the iterative patch below:

===================================================================
diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
index d990af67..db801800 100644
--- a/tools/memprof/humanize.lua
+++ b/tools/memprof/humanize.lua
@@ -61,20 +61,20 @@ function M.leak_info(heap_diff)
   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})
+    if info.dbytes > 0 then
+      table.insert(rest_heap, {line = line, dbytes = info.dbytes})
     end
   end
 
   table.sort(rest_heap, function(h1, h2)
-    return h1.hold_bytes > h2.hold_bytes
+    return h1.dbytes > h2.dbytes
   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].nalloc,
+      h.line, h.dbytes, heap_diff[h.line].nalloc,
       heap_diff[h.line].nfree
     ))
   end
diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua
index d3e944c7..9278b07c 100644
--- a/tools/memprof/process.lua
+++ b/tools/memprof/process.lua
@@ -8,7 +8,7 @@ function M.form_heap_diff(events, symbols)
   -- Auto resurrects source event lines for counting/reporting.
   local heap = setmetatable({}, {__index = function(t, line)
     t[line] = {
-      size_diff = 0,
+      dbytes = 0,
       nalloc = 0,
       nfree = 0,
     }
@@ -20,7 +20,7 @@ function M.form_heap_diff(events, symbols)
       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].dbytes = heap[ev_line].dbytes + event.alloc
         heap[ev_line].nalloc = heap[ev_line].nalloc + event.num
       end
     end
@@ -40,12 +40,12 @@ function M.form_heap_diff(events, symbols)
         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].dbytes = heap[ev_line].dbytes + heap_chunk.alloced
           heap[ev_line].nalloc = heap[ev_line].nalloc + 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].dbytes = heap[ev_line].dbytes - heap_chunk.freed
           heap[ev_line].nfree = heap[ev_line].nfree + heap_chunk.cnt
         end
       end
===================================================================

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

Also renamed field `cnt` to `count`.

===================================================================
diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
index a22c4fcb..12e2758f 100644
--- a/tools/memprof/parse.lua
+++ b/tools/memprof/parse.lua
@@ -48,14 +48,14 @@ local function link_to_previous(heap_chunk, e, nsize)
         loc = heap_chunk[3],
         alloced = 0,
         freed = 0,
-        cnt = 0,
+        count = 0,
       }
     end
     -- Save information about delta for memory heap.
     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
+    location_data.count = location_data.count + 1
   end
 end
 
diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua
index 69aeadbb..0bcb965b 100644
--- a/tools/memprof/process.lua
+++ b/tools/memprof/process.lua
@@ -41,12 +41,12 @@ function M.form_heap_delta(events, symbols)
 
         if (heap_chunk.alloced > 0) then
           dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.alloced
-          dheap[ev_line].nalloc = dheap[ev_line].nalloc + heap_chunk.cnt
+          dheap[ev_line].nalloc = dheap[ev_line].nalloc + heap_chunk.count
         end
 
         if (heap_chunk.freed > 0) then
           dheap[ev_line].dbytes = dheap[ev_line].dbytes - heap_chunk.freed
-          dheap[ev_line].nfree = dheap[ev_line].nfree + heap_chunk.cnt
+          dheap[ev_line].nfree = dheap[ev_line].nfree + heap_chunk.count
         end
       end
     end
===================================================================

> 
> Please, deal with your Hyde inside and fix the naming changeset-wide.

Fixed. Also tests are updated like the following:

===================================================================
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index 508834df..b4d66509 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -134,13 +134,13 @@ test:ok(free.INTERNAL.num == 102)
 
 -- Tests for leak-only option.
 -- See also https://github.com/tarantool/tarantool/issues/5812.
-local heap_diff = process.form_heap_diff(events, symbols)
-local tab_alloc_stats = heap_diff[form_source_line(21)]
-local str_alloc_stats = heap_diff[form_source_line(26)]
-test:ok(tab_alloc_stats.cnt_alloc == tab_alloc_stats.cnt_free)
-test:ok(tab_alloc_stats.size_diff == 0)
-test:ok(str_alloc_stats.cnt_alloc == str_alloc_stats.cnt_free)
-test:ok(str_alloc_stats.size_diff == 0)
+local heap_delta = process.form_heap_delta(events, symbols)
+local tab_alloc_stats = heap_delta[form_source_line(21)]
+local str_alloc_stats = heap_delta[form_source_line(26)]
+test:ok(tab_alloc_stats.nalloc == tab_alloc_stats.nfree)
+test:ok(tab_alloc_stats.dbytes == 0)
+test:ok(str_alloc_stats.nalloc == str_alloc_stats.nfree)
+test:ok(str_alloc_stats.dbytes == 0)
 
 -- Test for https://github.com/tarantool/tarantool/issues/5842.
 -- We are not interested in this report.
===================================================================

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

Added.

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

Fixed, see the iterative patch below:

===================================================================
diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua
index 83920be2..64acfd15 100644
--- a/tools/memprof/process.lua
+++ b/tools/memprof/process.lua
@@ -7,11 +7,11 @@ local symtab = require "utils.symtab"
 function M.form_heap_delta(events, symbols)
   -- Auto resurrects source event lines for counting/reporting.
   local dheap = setmetatable({}, {__index = function(t, line)
-    t[line] = {
+    rawset(t, line, {
       dbytes = 0,
       nalloc = 0,
       nfree = 0,
-    }
+    })
     return t[line]
   end})
===================================================================

> 
> > +      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 are not interested in location of realloc event, do we?
I suppose that if some reallocation (table for example) is performed,
that user doesn't interesting in leaks report where this object was
reallocated, but where it was declared (and so allocated).

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

No. It is local function to process reallocation and deallocation
events specific to heap delta aggregation. So, it is local to this
function exactly, not for the whole module.

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

Fixed, see the changes above.

> 
> > +end
> > +
> > +return M
> > -- 
> > 2.31.0
> > 
> 
> -- 
> Best regards,
> IM

[1]: https://github.com/tarantool/tarantool/issues/5994

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option
  2021-04-08 12:49 ` Sergey Ostanevich via Tarantool-patches
@ 2021-04-14 11:32   ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-04-14 11:32 UTC (permalink / raw)
  To: Sergey Ostanevich; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option
  2021-04-14 11:31   ` Sergey Kaplun via Tarantool-patches
@ 2021-04-14 13:12     ` Igor Munkin via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-04-14 13:12 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

Thanks for the fixes! Now the changes LGTM, I'll push them to the trunk
later.

On 14.04.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the review!
> 
> On 13.04.21, Igor Munkin wrote:
> > 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?
> 
> Yes, see [1].

Great, thanks!

> 
> Also I suggest to create a Code style guidelines for LuaJIT (as far as
> it is totally different from Tarantool's codestyle) for C and Lua.
> Looks like this guidelines is good for self-checking before sending
> patch for the review.
> Thoughts?

You've asked for this a couple of times already. I guess we need to
discuss this on our LuaJIT-related regular meeting.

> 
> > 
> > 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.
> 
> It is modelled on preamble, with post- replacing pre-.
> Changed to epilogue.
> 
> > 
> > > (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.
> 
> I'm a little bit confused here. How should I proceed the patch for the
> Tarantool repo? Usually Kirill or somebody just bumping version of a
> submodule with list of commits. Should I create the separate patch for
> this? Also, IINM, you asked me not to reference the issues in the commit
> message for Tarantool's luajit bump to avoid extra mentioning, because
> you anyway need to mention the issue during the bumping.

Sorry for confusing you. Frankly speaking, there is no strict rule and I
can do it by myself of course. However, if you need a rule of thumb I
suggest the following: whether you have changes in Tarantool repo, feel
free to add ChangeLog entry alongside with them, otherwise just put it
in the cover letter / patch (as you've done now).

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

<snipped>

> > > diff --git a/tools/memprof.lua b/tools/memprof.lua
> > > index 9f962085..c6c5f587 100644
> > > --- a/tools/memprof.lua
> > > +++ b/tools/memprof.lua

<snipped>

> > > @@ -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.
> 
> It was discussed with Sergos in the previous version. Also, AFAIR, Mons
> really asked to show this information always.

Then OK.

> 

<snipped>

> > > 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?
> 
> This module uses `print()` everywhere. <memprof.lua> uses stderr to
> write error messages at start and uses stdout for these messages.
> But as you can see `render()` function already uses just `print()`.
> So I prefer to use the same way of the output.

Oh, I didn't notice. Then fine, thanks for clarifying!

> 
> Side note: what do you mean by "silently"? How should I comment these
> changes?

There is not a single word in commit message regarding this. I guess it
was done unintentionally or with no reason. You've provided a one here,
so I'm totally OK with this change now.

> 

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

> > > +  -- 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 are not interested in location of realloc event, do we?
> I suppose that if some reallocation (table for example) is performed,
> that user doesn't interesting in leaks report where this object was
> reallocated, but where it was declared (and so allocated).

Well, this sounds valid. Anyway, we can fix this on demand, if somebody
needs more precise profiling. It would be nice to ask about it at first.

> 
> > 
> > > +  -- 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?
> 
> No. It is local function to process reallocation and deallocation
> events specific to heap delta aggregation. So, it is local to this
> function exactly, not for the whole module.

Yes, it's local here and has <dheap> and <symbols> as upvalues. But I
see no reason for this and you can implement it another way. Anyway,
this is not a problem and my comment is nothing more than doebka, so
let's move forward then.

> 

<snipped>

> > > -- 
> > > 2.31.0
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> [1]: https://github.com/tarantool/tarantool/issues/5994
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option
  2021-03-31 17:29 [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option Sergey Kaplun via Tarantool-patches
  2021-04-08 12:49 ` Sergey Ostanevich via Tarantool-patches
  2021-04-13  7:43 ` Igor Munkin via Tarantool-patches
@ 2021-04-14 14:34 ` Igor Munkin via Tarantool-patches
  2 siblings, 0 replies; 7+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-04-14 14:34 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

I've checked the series into tarantool branch in tarantool/luajit and
bumped a new version in master.

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
> (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):
> 
> ===================================================================
> ##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
> 

<snipped>

> -- 
> 2.31.0
> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-04-14 14:35 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-31 17:29 [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option 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
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox