From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 289524765E3 for ; Sun, 27 Dec 2020 10:17:43 +0300 (MSK) Date: Sun, 27 Dec 2020 10:16:56 +0300 From: Sergey Kaplun Message-ID: <20201227071656.GA9101@root> References: <03ac70e3bfb9bf7061ad71c1bac50ed3f8e853fc.1608907726.git.skaplun@tarantool.org> <20201226225651.GI5396@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201226225651.GI5396@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit v2 7/7] tools: introduce a memory profile parser List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.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 that displays > > human readable parsed data to a stdout. It calls 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 > > > > > 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 @@ > > > > > +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 > > > > > 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 @@ > > > > > +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! > > > + > > > > > +local reader = bufread.new(inputfile) > > +local symbols = symtab.parse(reader) > > +local events = memprof.parse(reader, symbols) > > Typo: Still mess with whitespace. My bad, fixed! > > > + > > > > > 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 . > > I see no such file. I guess you mean . Yes, thanks, fixed! > > > +-- > > > > > -- > > 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 . +-- The format spec can be found in . -- -- Major portions taken verbatim or adapted from the LuaVela. -- Copyright (C) 2015-2019 IPONWEB Ltd. =================================================================== -- Best regards, Sergey Kaplun