Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for the patch! LGTM, except the couple of nits below.

On 26.03.21, Sergey Kaplun wrote:
> Profiler parser output looks like the following:
> | ALLOCATIONS
> | @true_memleak.lua:6, line 9: 3	144	0
> | @true_memleak.lua:6, line 8: 2	41	0
> 
> Line of function definition is confusing for users and looks redundant.

Typo: s/is confusing for/confuses/.

> Also, these "magic numbers" may shock users.

Minor: Are you still talking about the line of function definition or
about the events/bytes here? It's a bit unclear from the wording.

> 
> This patch removes lines of function definitions from the output.
> Info about the line function definition is saved inside symbol info
> table by field `defined` (it can be used later for a user's custom
> parser). Moreover, explanatory words and signs are added to numbers
> for more verbose output.
> 
> After the patch, profiler parser output looks like the following:
> | ALLOCATIONS
> | @true_memleak.lua:9: 3 events	+144 bytes	-0 bytes
> | @true_memleak.lua:8: 2 events	+41 bytes	-0 bytes
> 
> Resolves tarantool/tarantool#5811
> Part of tarantool/tarantool#5657
> ---
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5811-user-friendly-memprof
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5811-user-friendly-memprof
> Issues:
> * https://github.com/tarantool/tarantool/issues/5811
> * https://github.com/tarantool/tarantool/issues/5657
> 
>  test/tarantool-tests/misclib-memprof-lapi.test.lua | 4 +++-
>  tools/memprof/humanize.lua                         | 2 +-
>  tools/utils/symtab.lua                             | 5 +++--
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 

<snipped>

> diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
> index 109a39db..2d5814c6 100644
> --- a/tools/memprof/humanize.lua
> +++ b/tools/memprof/humanize.lua
> @@ -20,7 +20,7 @@ function M.render(events, symbols)
>  
>    for i = 1, #ids do
>      local event = events[ids[i]]
> -    print(string.format("%s: %d\t%d\t%d",
> +    print(string.format("%s: %d events\t+%d bytes\t-%d bytes",

Minor: Heh, "events" is still plural even if <event.num> is 1. Feel free
to ignore (the current implementation might ease the postprocessing).

>        symtab.demangle(symbols, event.loc),
>        event.num,
>        event.alloc,
> diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua
> index 03aadbd5..6cfa1065 100644
> --- a/tools/utils/symtab.lua
> +++ b/tools/utils/symtab.lua
> @@ -25,7 +25,8 @@ local function parse_sym_lfunc(reader, symtab)
>    local sym_line = reader:read_uleb128()
>  
>    symtab[sym_addr] = {
> -    name = string_format("%s:%d", sym_chunk, sym_line),
> +    name = sym_chunk,
> +    defined = sym_line,

Minor: Since you split "name" into two components -- file name and line
the function is defined -- it looks natural to me to use the same names
as <debug.getinfo> does: source + linedefined. Anyway, feel free to
ignore, since the naming you chose is also fine.

>    }
>  end
>  

<snipped>

> -- 
> 2.31.0
> 

-- 
Best regards,
IM