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

[Sergey Kaplun <skaplun@tarantool.org>]

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