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

[Sergey Ostanevich <sergos@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 11684 bytes --]

LGTM.

Sergos

> On 27 Dec 2020, at 19:02, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> 
> Hi!
> 
> Thanks for the review.
> 
> On 27.12.20, Sergey Ostanevich wrote:
>> 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
>> ...
>> 
>> 
> 
> I suppose you want to use `popen` here to catch output?
> General practice is using `gsub` for now (see <test/utils.lua>).
> I fixed regexp to the following:
> 
> | local path = arg[0]:gsub("[^/]+%.test%.lua", "")
> | local path_suffix = "../tools/?.lua;"
> | package.path = ("%s%s;"):format(path, path_suffix)..package.path
> 
>>> +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?
> 
> No, I meant array part of table (not a hash part).
> I rewrote it to the following:
> | -- Preallocate table to avoid table array part reallocations.
> 
>>> +  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
> 
> Fixed.
> 
>>> +  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>
>> 
> 
> I also foggot to return `jit.on()` back in the end of test.
> Added.
> 
>>> 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?
> 
> Yes. It's looks better. Applied. Thank you!
> 
>> 
>>> +  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?
> 
> My bad. Done.
> 
>> 
>>> +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?
> 
> Hat-trick of buffer's misuse.
> Thanks you very much again! Fixed (see the iterative patch below).
> 
>> 
>>> +
>>> +  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
> 
> Fixed.
> 
>> 
>>> +  -- the end of the buffer.
>>> +  return reader._pos == reader._end
>>> +end
>> <snipped>
>> 
> 
> The iterative patch. Branch is force-pushed.
> ===================================================================
> diff --git a/test/misclib-memprof-lapi.test.lua b/test/misclib-memprof-lapi.test.lua
> index 2366c00..dd484f4 100755
> --- a/test/misclib-memprof-lapi.test.lua
> +++ b/test/misclib-memprof-lapi.test.lua
> @@ -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 = arg[0]:gsub("[^/]+%.test%.lua", "")
> local path_suffix = "../tools/?.lua;"
> -package.path = ("%s/%s;"):format(path, path_suffix)..package.path
> +package.path = ("%s%s;"):format(path, path_suffix)..package.path
> 
> local table_new = require "table.new"
> 
> @@ -23,7 +23,7 @@ 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.
> +  -- Preallocate table to avoid table array part reallocations.
>   local _ = table_new(100, 0)
> 
>   -- Want too see 100 objects here.
> @@ -38,7 +38,7 @@ local function payload()
> end
> 
> local function generate_output(filename)
> -  -- Clean up all garbage to avoid polution of free.
> +  -- Clean up all garbage to avoid pollution of free.
>   collectgarbage()
> 
>   local res, err = misc.memprof.start(filename)
> @@ -132,4 +132,5 @@ test:ok(check_alloc_report(alloc, 32, 25, 100))
> -- Collect all previous allocated objects.
> test:ok(free.INTERNAL.num == 102)
> 
> +jit.on()
> os.exit(test:check() and 0 or 1)
> diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
> index f4996f4..6dae22d 100644
> --- a/tools/memprof/parse.lua
> +++ b/tools/memprof/parse.lua
> @@ -19,10 +19,14 @@ local AEVENT_ALLOC = 1
> local AEVENT_FREE = 2
> local AEVENT_REALLOC = 3
> 
> +local AEVENT_MASK = 0x3
> +
> local ASOURCE_INT = lshift(1, 2)
> local ASOURCE_LFUNC = lshift(2, 2)
> local ASOURCE_CFUNC = lshift(3, 2)
> 
> +local ASOURCE_MASK = lshift(0x3, 2)
> +
> local M = {}
> 
> local function new_event(loc)
> @@ -35,9 +39,8 @@ local function new_event(loc)
>   }
> end
> 
> -local function link_to_previous(heap, e, oaddr)
> -  -- Memory at oaddr was allocated before we started tracking.
> -  local heap_chunk = heap[oaddr]
> +local function link_to_previous(heap_chunk, e)
> +  -- Memory at this chunk was allocated before we start tracking.
>   if heap_chunk then
>     -- Save Lua code location (line) by address (id).
>     e.primary[heap_chunk[2]] = heap_chunk[3]
> @@ -94,7 +97,7 @@ local function parse_realloc(reader, asource, events, heap)
>   e.free = e.free + osize
>   e.alloc = e.alloc + nsize
> 
> -  link_to_previous(heap, e, oaddr)
> +  link_to_previous(heap[oaddr], e)
> 
>   heap[oaddr] = nil
>   heap[naddr] = {nsize, id, loc}
> @@ -113,7 +116,7 @@ local function parse_free(reader, asource, events, heap)
>   e.num = e.num + 1
>   e.free = e.free + osize
> 
> -  link_to_previous(heap, e, oaddr)
> +  link_to_previous(heap[oaddr], e)
> 
>   heap[oaddr] = nil
> end
> @@ -131,7 +134,7 @@ end
> -- Splits event header into event type (aka aevent = allocation
> -- event) and event source (aka asource = allocation source).
> local function ev_header_split(evh)
> -  return band(evh, 0x3), band(evh, lshift(0x3, 2))
> +  return band(evh, AEVENT_MASK), band(evh, ASOURCE_MASK)
> end
> 
> local function parse_event(reader, events)
> diff --git a/tools/utils/bufread.lua b/tools/utils/bufread.lua
> index 873e06a..34bae9a 100644
> --- a/tools/utils/bufread.lua
> +++ b/tools/utils/bufread.lua
> @@ -22,7 +22,7 @@ local BUFFER_SIZE = 10 * 1024 * 1024
> local M = {}
> 
> ffi.cdef[[
> -  void *memcpy(void *, const void *, size_t);
> +  void *memmove(void *, const void *, size_t);
> 
>   typedef struct FILE_ FILE;
>   FILE *fopen(const char *, const char *);
> @@ -34,6 +34,8 @@ ffi.cdef[[
> local function _read_stream(reader, n)
>   local tail_size = reader._end - reader._pos
> 
> +  assert(n <= BUFFER_SIZE, "Internal buffer is large enough")
> +
>   if tail_size >= n then
>     -- Enough data to satisfy the request of n bytes.
>     return true
> @@ -41,16 +43,11 @@ local function _read_stream(reader, n)
> 
>   -- 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")
> +  local free_size = BUFFER_SIZE - tail_size
> 
>   if tail_size ~= 0 then
> -    ffi_C.memcpy(reader._buf, reader._buf + reader._pos, tail_size)
> +    ffi_C.memmove(reader._buf, reader._buf + reader._pos, tail_size)
>   end
> 
>   local bytes_read = ffi_C.fread(
> @@ -114,7 +111,7 @@ function M.eof(reader)
>   if sys_feof == 0 then
>     return false
>   end
> -  -- Otherwise return true only we have reached
> +  -- Otherwise return true only if we have reached
>   -- the end of the buffer.
>   return reader._pos == reader._end
> end
> ===================================================================
> 
> -- 
> Best regards,
> Sergey Kaplun


[-- Attachment #2: Type: text/html, Size: 143228 bytes --]