[Tarantool-patches] [PATCH luajit v2 7/7] tools: introduce a memory profile parser
Sergey Ostanevich
sergos at tarantool.org
Mon Dec 28 00:55:54 MSK 2020
LGTM.
Sergos
> On 27 Dec 2020, at 19:02, Sergey Kaplun <skaplun at 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 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20201228/c899698c/attachment.html>
More information about the Tarantool-patches
mailing list