[Tarantool-patches] [PATCH luajit v1] tools: introduce --leak-only memprof parser option
Sergey Ostanevich
sergos at tarantool.org
Mon Mar 29 18:50:58 MSK 2021
Updates are LGTM.
Although, I support Igor on his request to add some tests for alloc-free pairs detection.
Sergos
> On 17 Mar 2021, at 18:03, Sergey Kaplun <skaplun at tarantool.org> wrote:
>
> Hi!
>
> Thanks for the review!
>
> On 29.01.21, Sergey Ostanevich wrote:
>> Hi!
>> Thanks for the patch, some comments below.
>>
>> Regards,
>> Sergos
>>
>>
>>> On 25 Jan 2021, at 22:35, Sergey Kaplun <skaplun at tarantool.org> wrote:
>>>
>>> This patch adds a new --leak-only memory profiler parser option.
>>> When the parser runs with that option, it will report only lines
>>> (or "INTERNAL") that allocate and do not free some amount of bytes.
>>> The parser also reports the number of allocation and deallocation
>>> events related to each line.
>>> ---
>>>
>>> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-memprof-memleaks-option
>>> Testing branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-memprof-memleaks-option
>
> I removed the old branches and create issue[1]-related:
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5812-memprof-memleaks-option <https://github.com/tarantool/luajit/tree/skaplun/gh-5812-memprof-memleaks-option>
> Testing branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5812-memprof-memleaks-option <https://github.com/tarantool/tarantool/tree/skaplun/gh-5812-memprof-memleaks-option>
>
>>>
>>> Side note: I only update commit message thats why CI from the "old"
>>> commit.
>>> CI: https://gitlab.com/tarantool/tarantool/-/pipelines/246541599 <https://gitlab.com/tarantool/tarantool/-/pipelines/246541599>
>>>
>>> This patch is a result of offline discussion of memory profiler
>>> and its usage and feedback from Mons.
>>>
>>> I don't know - is it reasonable to reference [1] issue here or create a
>>> new one?
>>
>> I think it’s ok to mention [1]
>
> Updated commit message to the following:
>
> | tools: introduce --leak-only memprof parser option
> |
> | Memprof parser now adds postamble with the source lines of Lua chunks
> | (or "INTERNAL") that allocate and do not free some amount of bytes, when
> | profiler finishes. The parser also reports the number of allocation and
> | deallocation events related to each line.
> |
> | Also, this patch adds a new --leak-only memory profiler parser option.
> | When the parser runs with that option, it reports only leak
> | information.
> |
> | Resolves tarantool/tarantool#5812
>
> Also, I've added the following patch to be able to run memprof from
> Tarantool with leak-only option.
>
> ===================================================================
> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index 85dadd4..d2231ee 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua
> @@ -107,10 +107,14 @@ local function dump(inputfile)
> os.exit(0)
> end
>
> +local function dump_wrapped(...)
> + return dump(parseargs(...))
> +end
> +
> -- FIXME: this script should be application-independent.
> local args = {...}
> if #args == 1 and args[1] == "memprof" then
> - return dump
> + return dump_wrapped
> else
> - dump(parseargs(args))
> + dump_wrapped(args)
> end
> ===================================================================
>
> Now memprof parser can be called from Tarantool with --leak-only option
> like the following:
>
> | $ src/tarantool -e 'local memprof = require("memprof")(arg)' - --leak-only /tmp/memprof.bin
>
> Thoughts?
>
>>
>>>
>>> The example of output. Assuming we have file <format_concat.lua>:
>>> | 1 jit.off() -- more verbose reports
>>> | 2
>>> | 3 local function concat(a)
>>> | 4 local nstr = a.."a"
>>> | 5 return nstr
>>> | 6 end
>>> | 7
>>> | 8 local function format(a)
>>> | 9 local nstr = string.format("%sa", a)
>>> | 10 return nstr
>>> | 11 end
>>> | 12
>>> | 13 collectgarbage() -- cleanup
>>> | 14
>>> | 15 local binfile = "/tmp/memprof_"..(arg[0]):match("/([^/]*).lua")..".bin"
>>> | 16
>>> | 17 local st, err = misc.memprof.start(binfile)
>>> | 18 assert(st, err)
>>> | 19
>>> | 20 for i = 1, 10000 do
>>> | 21 local f = format(i)
>>> | 22 local c = concat(i)
>>> | 23 end
>>> | 24
>>> | 25 local st, err = misc.memprof.stop()
>>> | 26 assert(st, err)
>>> | 27
>>> | 28 os.exit()
>>>
>>> Parser's output without option:
>>> | ALLOCATIONS
>>> | @/home/burii/reports/demo_memprof/format_concat.lua:8, line 9: 19998 624322 0
>>> |
>>> | REALLOCATIONS
>>> |
>>> | DEALLOCATIONS
>>> | INTERNAL: 283 0 5602
>>> | Overrides:
>>> | @/home/burii/reports/demo_memprof/format_concat.lua:8, line 9
>>> |
>>> | @/home/burii/reports/demo_memprof/format_concat.lua:8, line 9: 2 0 98304
>>> | Overrides:
>>> | @/home/burii/reports/demo_memprof/format_concat.lua:8, line 9
>>>
>>> And with:
>>>
>>> | HEAP SUMMARY:
>>> | @/home/burii/reports/demo_memprof/format_concat.lua:8, line 9 holds 553214 bytes 19998 allocs, 283 frees
>>>
>>> Side note: The attentive reader will notice that we have 283+2 frees in
>>> the first case and 283 in the second.
>>> This is because --leak-only considers deallocation/reallocation events
>>> for which we know the source address. The default report aggregates
>>> these all deallocations into one. See `link_to_previous()` in
>>> <tools/memprof/parse.lua> for details.
>>>
>>> tools/memprof.lua | 23 +++++-----
>>> tools/memprof/humanize.lua | 91 +++++++++++++++++++++++++++++++++++++-
>>> tools/memprof/parse.lua | 20 +++++++--
>>> 3 files changed, 115 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/tools/memprof.lua b/tools/memprof.lua
>>> index 9f96208..5d09fbd 100644
>>> --- a/tools/memprof.lua
>>> +++ b/tools/memprof.lua
>>> @@ -37,6 +37,11 @@ Supported options are:
>>> os.exit(0)
>>> end
>>>
>>> +local leak_only = false
>>> +opt_map["leak-only"] = function()
>>
>> Should you add this into the SYNOPSIS part?
>
> Yes, thanks, see the iterative patch below:
>
> ===================================================================
> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index 5d09fbd..e1af99f 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua
> @@ -33,6 +33,7 @@ luajit-parse-memprof [options] memprof.bin
> Supported options are:
>
> --help Show this help and exit
> + --leak-only Report only leaks information
> ]]
> os.exit(0)
> end
> ===================================================================
>
>>
>>> + leak_only = true
>>> +end
>>> +
>>> -- Print error and exit with error status.
>>> local function opterror(...)
>>> stderr:write("luajit-parse-memprof.lua: ERROR: ", ...)
>>> @@ -94,19 +99,11 @@ local function dump(inputfile)
>>> local reader = bufread.new(inputfile)
>>> local symbols = symtab.parse(reader)
>>> local events = memprof.parse(reader, symbols)
>>> -
>>> - stdout:write("ALLOCATIONS", "\n")
>>> - view.render(events.alloc, symbols)
>>> - stdout:write("\n")
>>> -
>>> - stdout:write("REALLOCATIONS", "\n")
>>> - view.render(events.realloc, symbols)
>>> - stdout:write("\n")
>>> -
>>> - stdout:write("DEALLOCATIONS", "\n")
>>> - view.render(events.free, symbols)
>>> - stdout:write("\n")
>>> -
>>
>> Should the HEAP SUMMARY appear in the end of the ‘all’ report?
>> Otherwise the ‘_only’ in the option name is mesleading: show part of
>> the report, while it shows completely different one. Same for the
>> code below.
>
> Agree, fixed. Also, I renamed `all()` function name to the more verbose
> one:
>
> ===================================================================
> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index e1af99f..85dadd4 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua
> @@ -100,11 +100,10 @@ local function dump(inputfile)
> local reader = bufread.new(inputfile)
> local symbols = symtab.parse(reader)
> local events = memprof.parse(reader, symbols)
> - if leak_only then
> - view.leak_only(events, symbols)
> - else
> - view.all(events, symbols)
> + if not leak_only then
> + view.profile_info(events, symbols)
> end
> + view.leak_only(events, symbols)
> os.exit(0)
> end
>
> diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
> index bad0597..9e1bdf2 100644
> --- a/tools/memprof/humanize.lua
> +++ b/tools/memprof/humanize.lua
> @@ -42,7 +42,7 @@ function M.render(events, symbols)
> end
> end
>
> -function M.all(events, symbols)
> +function M.profile_info(events, symbols)
> print("ALLOCATIONS")
> M.render(events.alloc, symbols)
> print("")
> ===================================================================
>
>>
>>> + if leak_only then
>>> + view.leak_only(events, symbols)
>>> + else
>>> + view.all(events, symbols)
>>> + end
>>> os.exit(0)
>>> end
>>>
>>> diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
>>> index 109a39d..bad0597 100644
>>> --- a/tools/memprof/humanize.lua
>>> +++ b/tools/memprof/humanize.lua
>>> @@ -28,8 +28,8 @@ function M.render(events, symbols)
>>> ))
>>>
>>> local prim_loc = {}
>>> - for _, loc in pairs(event.primary) do
>>> - table.insert(prim_loc, symtab.demangle(symbols, loc))
>>> + for _, heap_chunk in pairs(event.primary) do
>>> + table.insert(prim_loc, symtab.demangle(symbols, heap_chunk.loc))
>>
>> The order of allocations is changed, was it incorrect in some way?
>
> The `event.primary` array is modified and contains now not only location
> line as a string, but a table with the line location in Lua chunk,
> plus info about amount of allocated and freed bytes.
>
>>
>>> end
>>> if #prim_loc ~= 0 then
>>> table.sort(prim_loc)
>>> @@ -42,4 +42,91 @@ function M.render(events, symbols)
>>> end
>>> end
>>>
>>> +function M.all(events, symbols)
>>> + print("ALLOCATIONS")
>>> + M.render(events.alloc, symbols)
>>> + print("")
>>> +
>>> + print("REALLOCATIONS")
>>> + M.render(events.realloc, symbols)
>>> + print("")
>>> +
>>> + print("DEALLOCATIONS")
>>> + M.render(events.free, symbols)
>>> + print("")
>>> +end
>>> +
>>> +function M.leak_only(events, symbols)
>>> + -- Auto resurrects source event lines for counting/reporting.
>>> + local heap = setmetatable({}, {__index = function(t, line)
>>> + t[line] = {
>>> + size = 0,
>>> + cnt_alloc = 0,
>>> + cnt_free = 0,
>>> + }
>>> + return t[line]
>>> + end})
>>> +
>>> + for _, event in pairs(events.alloc) do
>>> + if event.loc then
>>> + local ev_line = symtab.demangle(symbols, event.loc)
>>> +
>>> + heap[ev_line].size = heap[ev_line].size + event.alloc
>>> + if (event.alloc > 0) then
>>> + heap[ev_line].cnt_alloc = heap[ev_line].cnt_alloc + event.num
>>> + end
>>> + end
>>> + end
>>> +
>>> + -- Realloc and free events are pretty the same.
>>> + -- We don't interesting in aggregated alloc/free sizes for
>> aren’t interested
>
> Fixed, thanks! See the iterative patch below.
>
>>> + -- the event, but only for new and old size values inside
>>> + -- alloc-realloc-free chain. Assuming that we have
>>> + -- no collisions between different object addresses.
>>
>> I believe the address is source of the chunk {_|de|re}allocation, is there any
>> different one?
>
> I mean that, if some line allocated a few bytes in 0x4eadbeaf address,
> freed and the other one line allocated data at the same address later,
> it will be aggregated together.
>
>>
>>> + local function process_non_alloc_events(events_by_type)
>>> + for _, event in pairs(events_by_type) do
>>> + -- Realloc and free events always have "primary" key
>>> + -- that references table with rewrited memory
>>> + -- (may be empty).
>>> + for _, heap_chunk in pairs(event.primary) do
>>> + local ev_line = symtab.demangle(symbols, heap_chunk.loc)
>>> +
>>> + heap[ev_line].size = heap[ev_line].size + heap_chunk.alloced
>>> + if (heap_chunk.alloced > 0) then
>>
>> Can it be negative? If not - the above line can be put under this if also.
>
> No, it can't, see the iterative patch below:
>
> ===================================================================
> diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
> index 9e1bdf2..b8dbfbf 100644
> --- a/tools/memprof/humanize.lua
> +++ b/tools/memprof/humanize.lua
> @@ -71,15 +71,15 @@ function M.leak_only(events, symbols)
> if event.loc then
> local ev_line = symtab.demangle(symbols, event.loc)
>
> - heap[ev_line].size = heap[ev_line].size + event.alloc
> if (event.alloc > 0) then
> + heap[ev_line].size = heap[ev_line].size + event.alloc
> heap[ev_line].cnt_alloc = heap[ev_line].cnt_alloc + event.num
> end
> end
> end
>
> -- Realloc and free events are pretty the same.
> - -- We don't interesting in aggregated alloc/free sizes for
> + -- We aren't interested in aggregated alloc/free sizes for
> -- the event, but only for new and old size values inside
> -- alloc-realloc-free chain. Assuming that we have
> -- no collisions between different object addresses.
> @@ -91,13 +91,13 @@ function M.leak_only(events, symbols)
> for _, heap_chunk in pairs(event.primary) do
> local ev_line = symtab.demangle(symbols, heap_chunk.loc)
>
> - heap[ev_line].size = heap[ev_line].size + heap_chunk.alloced
> if (heap_chunk.alloced > 0) then
> + heap[ev_line].size = heap[ev_line].size + heap_chunk.alloced
> heap[ev_line].cnt_alloc = heap[ev_line].cnt_alloc + heap_chunk.cnt
> end
>
> - heap[ev_line].size = heap[ev_line].size - heap_chunk.freed
> if (heap_chunk.freed > 0) then
> + heap[ev_line].size = heap[ev_line].size - heap_chunk.freed
> heap[ev_line].cnt_free = heap[ev_line].cnt_free + heap_chunk.cnt
> end
> end
> ===================================================================
>
>>
>>> + heap[ev_line].cnt_alloc = heap[ev_line].cnt_alloc + heap_chunk.cnt
>>> + end
>>> +
>>> + heap[ev_line].size = heap[ev_line].size - heap_chunk.freed
>>> + if (heap_chunk.freed > 0) then
>>> + heap[ev_line].cnt_free = heap[ev_line].cnt_free + heap_chunk.cnt
>>> + end
>>> + end
>>> + end
>>> + end
>>> + process_non_alloc_events(events.realloc)
>>> + process_non_alloc_events(events.free)
>>> +
>>> + local rest_heap = {}
>>> + for line, info in pairs(heap) do
>>> + -- Report "INTERNAL" events inconsistencies for profiling
>>> + -- with enabled jit.
>>> + if info.size > 0 then
>>> + table.insert(rest_heap, {line = line, hold_bytes = info.size})
>>> + end
>>> + end
>>> +
>>> + table.sort(rest_heap, function(h1, h2)
>>> + return h1.hold_bytes > h2.hold_bytes
>>> + end)
>>> +
>>> + print("HEAP SUMMARY:")
>>> + for _, h in pairs(rest_heap) do
>>> + print(string.format(
>>> + "%s holds %d bytes %d allocs, %d frees",
>>> + h.line, h.hold_bytes, heap[h.line].cnt_alloc, heap[h.line].cnt_free
>>> + ))
>>> + end
>>> + print("")
>>> +end
>>> +
>>> return M
>>> diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
>>> index 6dae22d..374686c 100644
>>> --- a/tools/memprof/parse.lua
>>> +++ b/tools/memprof/parse.lua
>>> @@ -39,11 +39,23 @@ local function new_event(loc)
>>> }
>>> end
>>>
>>> -local function link_to_previous(heap_chunk, e)
>>> +local function link_to_previous(heap_chunk, e, nsize)
>>> -- Memory at this chunk was allocated before we start tracking.
>>> if heap_chunk then
>>> -- Save Lua code location (line) by address (id).
>>> - e.primary[heap_chunk[2]] = heap_chunk[3]
>>> + if e.primary[heap_chunk[2]] == nil then
>
> I don't like this line now, so I've updated it like the following:
>
> ===================================================================
> diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
> index 374686c..df10a45 100644
> --- a/tools/memprof/parse.lua
> +++ b/tools/memprof/parse.lua
> @@ -43,7 +43,7 @@ local function link_to_previous(heap_chunk, e, nsize)
> -- Memory at this chunk was allocated before we start tracking.
> if heap_chunk then
> -- Save Lua code location (line) by address (id).
> - if e.primary[heap_chunk[2]] == nil then
> + if not e.primary[heap_chunk[2]] then
> e.primary[heap_chunk[2]] = {
> loc = heap_chunk[3],
> alloced = 0,
> ===================================================================
>
>>> + e.primary[heap_chunk[2]] = {
>>> + loc = heap_chunk[3],
>>> + alloced = 0,
>>> + freed = 0,
>>> + cnt = 0,
>>> + }
>>> + end
>>> + -- Save memory diff heap information.
>>> + local location_data = e.primary[heap_chunk[2]]
>>> + location_data.alloced = location_data.alloced + nsize
>>> + location_data.freed = location_data.freed + heap_chunk[1]
>>> + location_data.cnt = location_data.cnt + 1
>>> end
>>> end
>>>
>>> @@ -97,7 +109,7 @@ local function parse_realloc(reader, asource, events, heap)
>>> e.free = e.free + osize
>>> e.alloc = e.alloc + nsize
>>>
>>> - link_to_previous(heap[oaddr], e)
>>> + link_to_previous(heap[oaddr], e, nsize)
>>>
>>> heap[oaddr] = nil
>>> heap[naddr] = {nsize, id, loc}
>>> @@ -116,7 +128,7 @@ local function parse_free(reader, asource, events, heap)
>>> e.num = e.num + 1
>>> e.free = e.free + osize
>>>
>>> - link_to_previous(heap[oaddr], e)
>>> + link_to_previous(heap[oaddr], e, 0)
>>>
>>> heap[oaddr] = nil
>>> end
>>> --
>>> 2.28.0
>>>
>>> [1]: https://github.com/tarantool/tarantool/issues/5657
>>
>
> [1]: https://github.com/tarantool/tarantool/issues/5812 <https://github.com/tarantool/tarantool/issues/5812>
>
> --
> Best regards,
> Sergey Kaplun
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20210329/2f2e63fa/attachment.htm>
More information about the Tarantool-patches
mailing list