Re: [Tarantool-patches] [PATCH luajit v1 11/11] profile: introduce profile parser

[Sergey Kaplun <skaplun@tarantool.org>]

Hi, Igor!

Thanks for the review!

On 25.12.20, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! Please consider my comments below.
> 
> On 16.12.20, Sergey Kaplun wrote:

<snipped>

> 
> General notes for all comments in the sources below:
> * Write it above the code to be commented
> * Start it with a capital letter
> * End it with a dot

Fixed.
Side note: It seemed to me that we decided offline, to transfer
the code from LuaVela with minimal changes in order to avoid conflicts in
case of patching. I am sorry for my misunderstanding.

> 
> > 
> > diff --git a/test/misclib-memprof-lapi.test.lua b/test/misclib-memprof-lapi.test.lua
> > new file mode 100755
> > index 0000000..e3663bb
> > --- /dev/null
> > +++ b/test/misclib-memprof-lapi.test.lua
> > @@ -0,0 +1,125 @@
> 
> <snipped>
> 
> > +local path = arg[0]:gsub('/[^/]+%.test%.lua', '')
> > +local parse_suffix = '../tools/parse_memprof/?.lua;'
> > +package.path = ('%s/%s;'):format(path, parse_suffix)..package.path
> 
> This is why I hate patching package.path in sources. There is a patchset
> where I move this suite to another place. Now I see that I need to patch
> this test. OK, we can do nothing with tests for now, so please add a
> FIXME note for these lines to fix the issue via environment variable.

Added, I suppose it should be done in case of this issue [1].

> 
> > +
> > +local table_new = require "table.new"
> > +
> > +local bufread = require "bufread"
> > +local memprof = require "parse_memprof"
> > +local symtab  = require "parse_symtab"
> 
> Typo: Something is wrong with the whitespace above.

Fixed.
Side note: As I see from the Lua developer style guide [2] "also, you
may use alignment". So it's not forbidden. But I take a look inside
LuaJIT <jit.*.lua> modules, for example, and there is no alignment
there. So I suppose we should use LuaJIT internal code style here for
consistency.

P.S: I think it's a good idea to add luacheck-like linter and
clang-format checker (if it is possible to avoid a huge diff) to safe
reviewer's and reviewee's time and formalize LuaJIT coding style guide.
Thoughts?

> 
> > +
> > +local TMP_BINFILE = arg[0]:gsub('[^/]+%.test%.lua', '%.%1.memprofdata.tmp.bin')
> > +local BAD_PATH    = arg[0]:gsub('[^/]+%.test%.lua', '%1/memprofdata.tmp.bin')
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +local tap = require('tap')
> > +
> > +local test = tap.test("misc-memprof-lapi")
> > +test:plan(6)
> 
> Why is the test initialized *here*?

Good question. Moved to the top.

> 
> > +
> 
> <snipped>
> 
> > +local function check_alloc_report(alloc, line, function_line, nevents)
> > +  assert(string.format("@%s:%d", arg[0], function_line) == alloc[line].name)
> > +  print(nevents, alloc[line].num)
> 
> Do you need this print?

No, thank you, removed.

> 
> > +  assert(alloc[line].num == nevents)
> > +  return true
> > +end
> > +
> > +-- Not a directory.
> > +local res, err = misc.memprof.start(BAD_PATH)
> > +test:ok(res == nil and err:match("Not a directory"))
> 
> This case can also check so-called "system-dependent error code", right?

Yes, I've already added corresponding test to the branch.

> 
> > +
> 
> <snipped>
> 
> > +local reader  = bufread.new(TMP_BINFILE)
> > +local symbols = symtab.parse(reader)
> > +local events  = memprof.parse(reader, symbols)
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +-- 1 event -- alocation of table by itself + 1 allocation
> > +-- of array part as far it bigger then LJ_MAX_COLOSIZE (16).
> > +test:ok(check_alloc_report(alloc, 21, 19, 2))
> > +-- 100 strings allocations.
> > +test:ok(check_alloc_report(alloc, 26, 19, 100))
> > +
> > +-- Collect all previous allocated objects.
> > +test:ok(free.INTERNAL.num == 102)
> 
> I guess you there is no need to name these magic numbers, but it's
> totally worth to leave a comment describing them.

Fixed.

> 
> > +
> > +os.exit(test:check() and 0 or 1)
> > diff --git a/tools/luajit-parse-memprof b/tools/luajit-parse-memprof
> > new file mode 100755
> > index 0000000..b9b16d7
> > --- /dev/null
> > +++ b/tools/luajit-parse-memprof
> 
> I believe it's better to implement runner to be installed to the system
> instead of the one to be used in sources. By the way, take a look on the
> way luajit.pc is installed.

Yes, as we discussed offline.

> 
> > @@ -0,0 +1,20 @@
> 
> <snipped>
> 
> > diff --git a/tools/parse_memprof/bufread.lua b/tools/parse_memprof/bufread.lua
> > new file mode 100644
> > index 0000000..d48d6e8
> > --- /dev/null
> > +++ b/tools/parse_memprof/bufread.lua
> > @@ -0,0 +1,143 @@
> 
> <snipped>
> 
> > +local ffi_C  = ffi.C
> > +local band   = bit.band
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +
> > +local function _read_stream(reader, n)
> > +  local free_size
> 
> Minor: It's better to declare this variable where it is used.

Fixed. Thanks!

> 
> > +  local tail_size = reader._end - reader._pos
> 
> <snipped>
> 
> > +function M.read_uleb128(reader)
> > +  local value = ffi.new('uint64_t', 0)
> > +  local shift = 0
> > +
> > +  repeat
> > +    local oct = M.read_octet(reader)
> > +
> > +    if oct == nil then
> > +      break
> > +    end
> 
> I guess this is an exceptional case, isn't it? If we hit this condition
> the ULEB128 value is misencoded, so it's worth to raise an error.

Done.

> 
> > +
> > +    -- Alas, bit library works only with 32-bit arguments:
> > +    local oct_u64 = ffi.new('uint64_t', band(oct, 0x7f))
> 
> Please do not use magic number in code.

Fixed. Thanks!

> 
> > +    value = value + oct_u64 * (2 ^ shift)
> > +    shift = shift + 7
> > +
> > +  until band(oct, 0x80) == 0
> 
> Please do not use magic number in code.

Fixed. Thanks!

> 
> > +
> > +  return tonumber(value)
> > +end
> 
> <snipped>
> 
> > diff --git a/tools/parse_memprof/main.lua b/tools/parse_memprof/main.lua
> > new file mode 100644
> > index 0000000..9a161b1
> > --- /dev/null
> > +++ b/tools/parse_memprof/main.lua
> > @@ -0,0 +1,104 @@
> 
> <snipped>
> 
> > +local bufread = require "bufread"
> > +local memprof = require "parse_memprof"
> > +local symtab  = require "parse_symtab"
> > +local view    = require "view_plain"
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> > +local stdout, stderr = io.stdout, io.stderr
> > +local _s = string
> 
> This variable looks excess: it is used once for the lookup below.

Fixed.

> 
> > +local match, gmatch = _s.match, _s.gmatch
> 
> <snipped>
> 
> > +-- Parse arguments.
> > +local function parseargs(args)
> > +  -- Process all option arguments.
> > +  args.argn = 1
> > +  repeat
> > +    local a = args[args.argn]
> > +    if not a then break end
> > +    local lopt, opt = match(a, "^%-(%-?)(.+)")
> > +    if not opt then break end
> > +    args.argn = args.argn + 1
> > +    if lopt == "" then
> > +      -- Loop through short options.
> > +      for o in gmatch(opt, ".") do parseopt(o, args) end
> 
> Please make this loop multiline.

Fixed (and if-then-end blocks too).

> 
> > +    else
> > +      -- Long option.
> > +      parseopt(opt, args)
> > +    end
> > +  until false
> > +
> > +  -- Check for proper number of arguments.
> > +  local nargs = #args - args.argn + 1
> > +  if nargs ~= 1 then
> > +    opt_map.help()
> > +  end
> > +
> > +  -- Translate a single input file.
> > +  -- TODO: Handle multiple files?
> > +  return args[args.argn]
> > +end
> > +
> > +local inputfile = parseargs{...}
> > +
> > +local reader  = bufread.new(inputfile)
> > +local symbols = symtab.parse(reader)
> > +local events  = memprof.parse(reader, symbols)
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > diff --git a/tools/parse_memprof/parse_memprof.lua b/tools/parse_memprof/parse_memprof.lua
> > new file mode 100644
> > index 0000000..dc56fed
> > --- /dev/null
> > +++ b/tools/parse_memprof/parse_memprof.lua
> > @@ -0,0 +1,195 @@
> 
> <snipped>
> 
> > +local bit    = require 'bit'
> > +local band   = bit.band
> > +local lshift = bit.lshift
> > +
> > +local string_format = string.format
> > +
> > +local LJM_MAGIC           = 'ljm'
> > +local LJM_CURRENT_VERSION = 2
> > +
> > +local LJM_EPILOGUE_HEADER = 0x80
> > +
> > +local AEVENT_ALLOC   = 1
> > +local AEVENT_FREE    = 2
> > +local AEVENT_REALLOC = 3
> > +
> > +local ASOURCE_INT   = lshift(1, 2)
> > +local ASOURCE_LFUNC = lshift(2, 2)
> > +local ASOURCE_CFUNC = lshift(3, 2)
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +local function link_to_previous(heap, e, oaddr)
> > +  -- memory at oaddr was allocated before we started tracking:
> > +  if heap[oaddr] then
> > +    e.primary[heap[oaddr][2]] = heap[oaddr][3]
> 
> Please leave the comment that the key is id and the value is location.

Done.

> 
> > +  end
> > +end
> > +
> > +local function parse_location(reader, asource)
> 
> What about introducing another function for it?
> | local function id_location(addr, line)
> |   return string_format('f%#xl%d', addr, line), {
> |     addr = addr,
> |     line = line,
> |   }
> | end
> |
> | local function parse_location(reader, asource)
> |   if asource == ASOURCE_INT then
> |     return id_location(0, 0)
> |   elseif asource == ASOURCE_CFUNC then
> |     return id_location(reader:read_uleb128(), 0)
> |   elseif asource == ASOURCE_LFUNC then
> |     return id_location(reader:read_uleb128(), reader:read_uleb128())
> |   end
> |   error('Unknown asource '..asource)
> | end

Thanks, applied.

> 
> <snipped>
> 
> > +  local e = events[id]
> > +  e.num   = e.num + 1
> > +  e.alloc = e.alloc + nsize
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +  local e = events[id]
> > +  e.num   = e.num + 1
> > +  e.free  = e.free + osize
> > +  e.alloc = e.alloc + nsize
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +  local e = events[id]
> > +  e.num   = e.num + 1
> > +  e.free  = e.free + osize
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +local parsers = {
> > +  [AEVENT_ALLOC]   = {evname =   'alloc', parse = parse_alloc},
> > +  [AEVENT_REALLOC] = {evname = 'realloc', parse = parse_realloc},
> > +  [AEVENT_FREE]    = {evname =    'free', parse = parse_free},
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +}
> 
> <snipped>
> 
> > +local function ev_header_is_epilogue(evh)
> > +  return evh == LJM_EPILOGUE_HEADER
> > +end
> 
> This function is excess. Just use this comparison in the condition.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +  local magic   = reader:read_octets(3)
> > +  local version = reader:read_octets(1)
> > +  -- dummy-consume reserved bytes
> > +  local _       = reader:read_octets(3)
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > diff --git a/tools/parse_memprof/parse_symtab.lua b/tools/parse_memprof/parse_symtab.lua
> > new file mode 100644
> > index 0000000..54e9337
> > --- /dev/null
> > +++ b/tools/parse_memprof/parse_symtab.lua
> > @@ -0,0 +1,88 @@
> 
> <snipped>
> 
> > +local band          = bit.band
> > +local string_format = string.format
> > +
> > +local LJS_MAGIC           = 'ljs'
> > +local LJS_CURRENT_VERSION = 2
> > +local LJS_EPILOGUE_HEADER = 0x80
> > +local LJS_SYMTYPE_MASK    = 0x03
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +-- Parse a single entry in a symtab: lfunc symbol
> > +local function parse_sym_lfunc(reader, symtab)
> > +  local sym_addr  = reader:read_uleb128()
> > +  local sym_chunk = reader:read_string()
> > +  local sym_line  = reader:read_uleb128()
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +function M.parse(reader)
> > +  local symtab   = {}
> > +  local magic    = reader:read_octets(3)
> > +  local version  = reader:read_octets(1)
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

> 
> > +
> 
> <snipped>
> 
> > +
> > +  while not reader:eof() do
> > +    local header   = reader:read_octet()
> > +    local is_final = band(header, LJS_EPILOGUE_HEADER) ~= 0
> 
> Typo: Something is wrong with the whitespace above.

Fixed.

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

[1]: https://github.com/tarantool/tarantool/issues/4862
[2]: https://www.tarantool.io/en/doc/2.5/dev_guide/lua_style_guide/

-- 
Best regards,
Sergey Kaplun