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