Re: [Tarantool-patches] [PATCH luajit v2 7/7] tools: introduce a memory profile parser

[Igor Munkin <imun@tarantool.org>]

Sergey,

Thanks for the patch! LGTM, except the several nits below.

On 25.12.20, Sergey Kaplun wrote:
> This patch adds a parser for binary data dumped via the memory profiler. It is
> a set of the following Lua modules:
> * utils/bufread.lua: read binary data from a file.
> * utils/symtab.lua: symbol table decode functions
> * memprof/parse.lua: decode the memory profiler event stream
> * memprof/humanize.lua: display decoded data in human readable format
> * memprof.lua: Lua script to display data
> 
> There is also a stand-alone bash script <luajit-parse-memprof> that displays
> human readable parsed data to a stdout. It calls <memprof.lua> with a
> corresponding LUA_PATH.
> 
> Part of tarantool/tarantool#5442
> Part of tarantool/tarantool#5490
> ---
> 
> Changes in v2:
>   - Add (un)?install sections in Makefile
>   - Modify bash script correspondingly.
>   - Change Lua modules layout.
>   - Adjusted test. Check that errno returns in case of error is added.
>   - Code clean up.
> 
>  Makefile                           |  39 +++++-
>  test/misclib-memprof-lapi.test.lua | 135 +++++++++++++++++++++
>  tools/luajit-parse-memprof         |   9 ++
>  tools/memprof.lua                  | 109 +++++++++++++++++
>  tools/memprof/humanize.lua         |  45 +++++++
>  tools/memprof/parse.lua            | 188 +++++++++++++++++++++++++++++
>  tools/utils/bufread.lua            | 147 ++++++++++++++++++++++
>  tools/utils/symtab.lua             |  89 ++++++++++++++
>  8 files changed, 757 insertions(+), 4 deletions(-)
>  create mode 100755 test/misclib-memprof-lapi.test.lua
>  create mode 100755 tools/luajit-parse-memprof
>  create mode 100644 tools/memprof.lua
>  create mode 100644 tools/memprof/humanize.lua
>  create mode 100644 tools/memprof/parse.lua
>  create mode 100644 tools/utils/bufread.lua
>  create mode 100644 tools/utils/symtab.lua
> 
> diff --git a/Makefile b/Makefile
> index 4a56917..ba4aa2f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -37,6 +37,9 @@ INSTALL_INC=   $(DPREFIX)/include/luajit-$(MAJVER).$(MINVER)
>  
>  INSTALL_LJLIBD= $(INSTALL_SHARE)/luajit-$(VERSION)
>  INSTALL_JITLIB= $(INSTALL_LJLIBD)/jit
> +INSTALL_UTILSLIB= $(INSTALL_LJLIBD)/utils
> +INSTALL_MEMPROFLIB= $(INSTALL_LJLIBD)/memprof

Minor: It's better to use $(INSALL_TOOLSLIB) here, isn't it?

> +INSTALL_TOOLSLIB= $(INSTALL_LJLIBD)
>  INSTALL_LMODD= $(INSTALL_SHARE)/lua
>  INSTALL_LMOD= $(INSTALL_LMODD)/$(ABIVER)
>  INSTALL_CMODD= $(INSTALL_LIB)/lua

<snipped>

> diff --git a/test/misclib-memprof-lapi.test.lua b/test/misclib-memprof-lapi.test.lua
> new file mode 100755
> index 0000000..e02c6fa
> --- /dev/null
> +++ b/test/misclib-memprof-lapi.test.lua
> @@ -0,0 +1,135 @@

<snipped>

> +local function check_alloc_report(alloc, line, function_line, nevents)
> +  assert(string.format("@%s:%d", arg[0], function_line) == alloc[line].name)
> +  assert(alloc[line].num == nevents, ("got=%d, ecpected=%d"):format(

Typo: s/ecpected/expected/.

> +    alloc[line].num,
> +    nevents
> +  ))
> +  return true
> +end

<snipped>

> diff --git a/tools/luajit-parse-memprof b/tools/luajit-parse-memprof
> new file mode 100755
> index 0000000..c814301
> --- /dev/null
> +++ b/tools/luajit-parse-memprof
> @@ -0,0 +1,9 @@
> +#!/bin/bash
> +#
> +# Launcher for memprof parser.
> +
> +# This two variables are replaced on installing.
> +TOOL_DIR=$(dirname `readlink -f $0`)
> +LUAJIT_BIN=$TOOL_DIR/../src/luajit
> +
> +LUA_PATH="$TOOL_DIR/?.lua;;" $LUAJIT_BIN $TOOL_DIR/memprof.lua $@
> diff --git a/tools/memprof.lua b/tools/memprof.lua
> new file mode 100644
> index 0000000..7476757
> --- /dev/null
> +++ b/tools/memprof.lua
> @@ -0,0 +1,109 @@

<snipped>

> +local bufread = require "utils.bufread"
> +local memprof = require "memprof.parse"
> +local symtab  = require "utils.symtab"
> +local view    = require "memprof.humanize"

Typo: Still mess with whitespace.

> +

<snipped>

> +local reader  = bufread.new(inputfile)
> +local symbols = symtab.parse(reader)
> +local events  = memprof.parse(reader, symbols)

Typo: Still mess with whitespace.

> +

<snipped>

> diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua
> new file mode 100644
> index 0000000..f3e5e31
> --- /dev/null
> +++ b/tools/utils/symtab.lua
> @@ -0,0 +1,89 @@
> +-- Parser of LuaJIT's symtab binary stream.
> +-- The format spec can be found in <src/lj_symtab.h>.

I see no such file. I guess you mean <src/lj_memprof.h>.

> +--

<snipped>

> -- 
> 2.28.0
> 

-- 
Best regards,
IM