[Tarantool-patches] [PATCH luajit v2 7/7] tools: introduce a memory profile parser
Sergey Kaplun
skaplun at tarantool.org
Sun Dec 27 19:02:13 MSK 2020
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 at 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
More information about the Tarantool-patches
mailing list