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

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the patch!
See my 7 comments below.

Sergos


> On 25 Dec 2020, at 18:26, Sergey Kaplun <skaplun@tarantool.org> wrote:
<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 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +
> +local test = tap.test("misc-memprof-lapi")
> +test:plan(9)
> +
> +jit.off()
> +jit.flush()
> +
> +-- FIXME: Launch tests with LUA_PATH enviroment variable.
> +local path = arg[0]:gsub('/[^/]+%.test%.lua', ‘’)

I believe it won’t work well for some cases, such as

tarantool> arg[0]
---
- void.test.lua
...

tarantool> arg[0]:gsub('/[^/]+%.test%.lua', '')
---
- void.test.lua
- 0
...

Alternative is:

tarantool> os.execute('dirname '..arg[0])
.
---
- 0
...


> +local path_suffix = '../tools/?.lua;'
> +package.path = ('%s/%s;'):format(path, path_suffix)..package.path
> +
> +local table_new = require "table.new"
> +
> +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 function payload()
> +  -- Preallocate table to avoid array part reallocations.
                                         ^parts?
> +  local _ = table_new(100, 0)
> +
> +  -- Want too see 100 objects here.
> +  for i = 1, 100 do
> +    -- Try to avoid crossing with "test" module objects.
> +    _[i] = "memprof-str-"..i
> +  end
> +
> +  _ = nil
> +  -- VMSTATE == GC, reported as INTERNAL.
> +  collectgarbage()
> +end
> +
> +local function generate_output(filename)
> +  -- Clean up all garbage to avoid polution of free.
                                     pollution
> +  collectgarbage()
> +
> +  local res, err = misc.memprof.start(filename)
> +  -- Should start succesfully.
> +  assert(res, err)
> +
> +  payload()
> +
> +  res, err = misc.memprof.stop()
> +  -- Should stop succesfully.
> +  assert(res, err)
> +end

<snipped>

> diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
> new file mode 100644
> index 0000000..f4996f4
> --- /dev/null
> +++ b/tools/memprof/parse.lua

<snipped>

> +local function link_to_previous(heap, e, oaddr)
> +  -- Memory at oaddr was allocated before we started tracking.
> +  local heap_chunk = heap[oaddr]

Do you need two args for this? Can you just pass the heap[oaddr] instead?

> +  if heap_chunk then
> +    -- Save Lua code location (line) by address (id).
> +    e.primary[heap_chunk[2]] = heap_chunk[3]
> +  end
> +end
> +

<snipped>

> +local function ev_header_split(evh)
> +  return band(evh, 0x3), band(evh, lshift(0x3, 2))

Should you intorduce masks along with AEVENT/ASOURCE to avoid these
magic numbers?

> +end
> +

<snipped>

> diff --git a/tools/utils/bufread.lua b/tools/utils/bufread.lua

<snipped>

> +
> +local function _read_stream(reader, n)
> +  local tail_size = reader._end - reader._pos
> +
> +  if tail_size >= n then
> +    -- Enough data to satisfy the request of n bytes.
> +    return true
> +  end
> +
> +  -- Otherwise carry tail_size bytes from the end of the buffer
> +  -- to the start and fill up free_size bytes with fresh data.
> +  -- tail_size < n <= free_size (see assert below) ensures that
> +  -- we don't copy overlapping memory regions.
> +  -- reader._pos == 0 means filling buffer for the first time.
> +
> +  local free_size = reader._pos > 0 and reader._pos or n
> +
> +  assert(n <= free_size, "Internal buffer is large enough")

Does it mean I will have a fail in case _pos is less that half of the
buffer and n is more than the tail_size? 
Which means I can use only half of the buffer?

> +
> +  if tail_size ~= 0 then
> +    ffi_C.memcpy(reader._buf, reader._buf + reader._pos, tail_size)
> +  end
> +
> +  local bytes_read = ffi_C.fread(
> +    reader._buf + tail_size, 1, free_size, reader._file
> +  )
> +
> +  reader._pos = 0
> +  reader._end = tail_size + bytes_read
> +
> +  return reader._end - reader._pos >= n
> +end
> +

<snipped>

> +function M.eof(reader)
> +  local sys_feof = ffi_C.feof(reader._file)
> +  if sys_feof == 0 then
> +    return false
> +  end
> +  -- Otherwise return true only we have reached
                                  ^^ if we

> +  -- the end of the buffer.
> +  return reader._pos == reader._end
> +end
<snipped>