From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 61F034765E3 for ; Sun, 27 Dec 2020 19:03:00 +0300 (MSK) Date: Sun, 27 Dec 2020 19:02:13 +0300 From: Sergey Kaplun Message-ID: <20201227160213.GC14702@root> References: <03ac70e3bfb9bf7061ad71c1bac50ed3f8e853fc.1608907726.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH luajit v2 7/7] tools: introduce a memory profile parser List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich Cc: tarantool-patches@dev.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 wrote: > > > 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 ). 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 > > > 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 > > > > > +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 > > + > > > > > +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 > > + > > > > > diff --git a/tools/utils/bufread.lua b/tools/utils/bufread.lua > > > > > + > > +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 > > + > > > > > +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 > > 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