[Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option
Sergey Kaplun
skaplun at tarantool.org
Wed Apr 14 14:31:41 MSK 2021
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
More information about the Tarantool-patches
mailing list