From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 8AB104765E3 for ; Sun, 27 Dec 2020 16:24:23 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) From: Sergey Ostanevich In-Reply-To: <03ac70e3bfb9bf7061ad71c1bac50ed3f8e853fc.1608907726.git.skaplun@tarantool.org> Date: Sun, 27 Dec 2020 16:24:21 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <03ac70e3bfb9bf7061ad71c1bac50ed3f8e853fc.1608907726.git.skaplun@tarantool.org> 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 Kaplun Cc: tarantool-patches@dev.tarantool.org 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 =3D require('tap') > + > +local test =3D tap.test("misc-memprof-lapi") > +test:plan(9) > + > +jit.off() > +jit.flush() > + > +-- FIXME: Launch tests with LUA_PATH enviroment variable. > +local path =3D arg[0]:gsub('/[^/]+%.test%.lua', =E2=80=98=E2=80=99) I believe it won=E2=80=99t 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 ... > +local path_suffix =3D '../tools/?.lua;' > +package.path =3D ('%s/%s;'):format(path, path_suffix)..package.path > + > +local table_new =3D require "table.new" > + > +local bufread =3D require "utils.bufread" > +local memprof =3D require "memprof.parse" > +local symtab =3D require "utils.symtab" > + > +local TMP_BINFILE =3D arg[0]:gsub('[^/]+%.test%.lua', = '%.%1.memprofdata.tmp.bin') > +local BAD_PATH =3D arg[0]:gsub('[^/]+%.test%.lua', = '%1/memprofdata.tmp.bin') > + > +local function payload() > + -- Preallocate table to avoid array part reallocations. ^parts? > + local _ =3D table_new(100, 0) > + > + -- Want too see 100 objects here. > + for i =3D 1, 100 do > + -- Try to avoid crossing with "test" module objects. > + _[i] =3D "memprof-str-"..i > + end > + > + _ =3D nil > + -- VMSTATE =3D=3D GC, reported as INTERNAL. > + collectgarbage() > +end > + > +local function generate_output(filename) > + -- Clean up all garbage to avoid polution of free. pollution > + collectgarbage() > + > + local res, err =3D misc.memprof.start(filename) > + -- Should start succesfully. > + assert(res, err) > + > + payload() > + > + res, err =3D misc.memprof.stop() > + -- Should stop succesfully. > + assert(res, err) > +end > 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 =3D heap[oaddr] Do you need two args for this? Can you just pass the heap[oaddr] = instead? > + if heap_chunk then > + -- Save Lua code location (line) by address (id). > + e.primary[heap_chunk[2]] =3D 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? > +end > + > diff --git a/tools/utils/bufread.lua b/tools/utils/bufread.lua > + > +local function _read_stream(reader, n) > + local tail_size =3D reader._end - reader._pos > + > + if tail_size >=3D 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 <=3D free_size (see assert below) ensures that > + -- we don't copy overlapping memory regions. > + -- reader._pos =3D=3D 0 means filling buffer for the first time. > + > + local free_size =3D reader._pos > 0 and reader._pos or n > + > + assert(n <=3D 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?=20 Which means I can use only half of the buffer? > + > + if tail_size ~=3D 0 then > + ffi_C.memcpy(reader._buf, reader._buf + reader._pos, tail_size) > + end > + > + local bytes_read =3D ffi_C.fread( > + reader._buf + tail_size, 1, free_size, reader._file > + ) > + > + reader._pos =3D 0 > + reader._end =3D tail_size + bytes_read > + > + return reader._end - reader._pos >=3D n > +end > + > +function M.eof(reader) > + local sys_feof =3D ffi_C.feof(reader._file) > + if sys_feof =3D=3D 0 then > + return false > + end > + -- Otherwise return true only we have reached ^^ if we > + -- the end of the buffer. > + return reader._pos =3D=3D reader._end > +end