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

[Sergey Kaplun <skaplun@tarantool.org>]

Igor,

Thanks for the review!

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

Yes, fixed.

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

Thanks! Fixed!

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

Sorry, fixed!

> 
> > +
> 
> <snipped>
> 
> > +local reader  = bufread.new(inputfile)
> > +local symbols = symtab.parse(reader)
> > +local events  = memprof.parse(reader, symbols)
> 
> Typo: Still mess with whitespace.

My bad, fixed!

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

Yes, thanks, fixed!

> 
> > +--
> 
> <snipped>
> 
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Best regards,
> IM

Also fixed mess with quotes for strings.
See the iterative patch below. Branch is force-pushed.

===================================================================
diff --git a/Makefile b/Makefile
index ba4aa2f..61967df 100644
--- a/Makefile
+++ b/Makefile
@@ -37,9 +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
 INSTALL_TOOLSLIB= $(INSTALL_LJLIBD)
+INSTALL_UTILSLIB= $(INSTALL_TOOLSLIB)/utils
+INSTALL_MEMPROFLIB= $(INSTALL_TOOLSLIB)/memprof
 INSTALL_LMODD= $(INSTALL_SHARE)/lua
 INSTALL_LMOD= $(INSTALL_LMODD)/$(ABIVER)
 INSTALL_CMODD= $(INSTALL_LIB)/lua
diff --git a/test/misclib-memprof-lapi.test.lua b/test/misclib-memprof-lapi.test.lua
index e02c6fa..2366c00 100755
--- a/test/misclib-memprof-lapi.test.lua
+++ b/test/misclib-memprof-lapi.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 
-local tap = require('tap')
+local tap = require("tap")
 
 local test = tap.test("misc-memprof-lapi")
 test:plan(9)
@@ -9,9 +9,9 @@ jit.off()
 jit.flush()
 
 -- FIXME: Launch tests with LUA_PATH enviroment variable.
-local path = arg[0]:gsub('/[^/]+%.test%.lua', '')
-local path_suffix = '../tools/?.lua;'
-package.path = ('%s/%s;'):format(path, path_suffix)..package.path
+local path = arg[0]:gsub("/[^/]+%.test%.lua", "")
+local path_suffix = "../tools/?.lua;"
+package.path = ("%s/%s;"):format(path, path_suffix)..package.path
 
 local table_new = require "table.new"
 
@@ -19,8 +19,8 @@ local bufread = require "utils.bufread"
 local memprof = require "memprof.parse"
 local symtab = require "utils.symtab"
 
-local TMP_BINFILE = arg[0]:gsub('[^/]+%.test%.lua', '%.%1.memprofdata.tmp.bin')
-local BAD_PATH = arg[0]:gsub('[^/]+%.test%.lua', '%1/memprofdata.tmp.bin')
+local TMP_BINFILE = arg[0]:gsub("[^/]+%.test%.lua", "%.%1.memprofdata.tmp.bin")
+local BAD_PATH = arg[0]:gsub("[^/]+%.test%.lua", "%1/memprofdata.tmp.bin")
 
 local function payload()
   -- Preallocate table to avoid array part reallocations.
@@ -73,7 +73,7 @@ end
 
 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(
+  assert(alloc[line].num == nevents, ("got=%d, expected=%d"):format(
     alloc[line].num,
     nevents
   ))
diff --git a/tools/memprof.lua b/tools/memprof.lua
index 7476757..92d192e 100644
--- a/tools/memprof.lua
+++ b/tools/memprof.lua
@@ -12,8 +12,8 @@
 
 local bufread = require "utils.bufread"
 local memprof = require "memprof.parse"
-local symtab  = require "utils.symtab"
-local view    = require "memprof.humanize"
+local symtab = require "utils.symtab"
+local view = require "memprof.humanize"
 
 local stdout, stderr = io.stdout, io.stderr
 local match, gmatch = string.match, string.gmatch
@@ -92,9 +92,9 @@ end
 
 local inputfile = parseargs{...}
 
-local reader  = bufread.new(inputfile)
+local reader = bufread.new(inputfile)
 local symbols = symtab.parse(reader)
-local events  = memprof.parse(reader, symbols)
+local events = memprof.parse(reader, symbols)
 
 stdout:write("ALLOCATIONS", "\n")
 view.render(events.alloc, symbols)
diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua
index f3e5e31..03aadbd 100644
--- a/tools/utils/symtab.lua
+++ b/tools/utils/symtab.lua
@@ -1,5 +1,5 @@
 -- Parser of LuaJIT's symtab binary stream.
--- The format spec can be found in <src/lj_symtab.h>.
+-- The format spec can be found in <src/lj_memprof.h>.
 --
 -- Major portions taken verbatim or adapted from the LuaVela.
 -- Copyright (C) 2015-2019 IPONWEB Ltd.
===================================================================

-- 
Best regards,
Sergey Kaplun