From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 2F9042ACFC1; Tue, 23 May 2023 15:41:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2F9042ACFC1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1684845665; bh=/pByznJ1kFeY9wAudE4ulQ8EiFqcxkOStKgvT1vZKPg=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=uGQ1R5hrAVc/1FZE/U7FDzJaplKdrZgtzi7is8kV2ObPhvQMQVbM5pnc9gdy9SizZ +ay7UAKrhpy6H1EPACBlP+wveI84RcgFVTvz0rVUZRhcXuQiC8MJ6xLGriV9a+si/R 5bF7kPI1OGQy+VxO61n/g+3MQrJs/PvrDsvWYI88= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 598512ACFC1 for ; Tue, 23 May 2023 15:41:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 598512ACFC1 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1q1RK1-0004bL-IP; Tue, 23 May 2023 15:41:03 +0300 Date: Tue, 23 May 2023 15:36:55 +0300 To: Maxim Kokryashkin Message-ID: References: <20230518114927.277888-1-m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230518114927.277888-1-m.kokryashkin@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9318AAE2601AA39B849C248043F9511BC1BBFE9B9F88C6A2500894C459B0CD1B94BF5291DE4DE6C3ED98FCBE37CFF8C0B23C64D8C0C9336E57BC1A8A20AE16A52 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70ED3881ADD6CEF6AEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375549E03BCBF89E018638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D843277F2CD61AADF219EB27A7180334DA117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA1C5B227563B4AFA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520CCD848CCB6FE560CF04B652EEC242312D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B613439FA09F3DCB32089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5E2732CDCA48B50514FB0C4885087D67B038E8D09A7CF0F94F87CCE6106E1FC07E67D4AC08A07B9B0CE135D2742255B35CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF5F1D9860BDBDAE241654B27C0F9B109361F26DD010D9648BDEDFD5C2173E7EEE45D21F2230575A0FD5191E2618EE498C6A3DACD8EEB3486C6A341B08F40EEDCBF4E8A8FB6BF8EBF5 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojzY8FQurh3bCjGqwTVyLt7g== X-DA7885C5: CF72D99D86B541B5DE6AB59F29739FB250D5A8F3C71D4146A12033C093795253262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986B5CDA8DA6B65084023A08A66DE87BE0190FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] sysprof: improve parser's memory footprint X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim! Thanks for the patch! LGTM, with some minor comments below. On 18.05.23, Maxim Kokryashkin wrote: > This patch reduces sysprof's parser memory footprint, > by avoiding reading all callchains before collapsing them. > Instead of it, parser merges stacks immediately after > reading them and stores counts in a lua table. > > Also, it fixes a bug in the AVL-tree implementation, > which produced unnecessary inserts of values into nodes. Should it be any test for this? Also, may be this should be done in the separate commit (not patch?). > --- > Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-noticket-sysprof-parser-refactoring > PR: https://github.com/tarantool/tarantool/pull/8670 > > NB: CI is red in LuaJIT repo because this patch requires changes in the > tarantool repo, so please refer to CI runs in PR. > > tools/CMakeLists.txt | 2 - > tools/sysprof.lua | 27 +------- > tools/sysprof/collapse.lua | 124 ------------------------------------ > tools/sysprof/parse.lua | 125 ++++++++++++++++++++++++++----------- > tools/utils/avl.lua | 2 +- > tools/utils/symtab.lua | 2 +- > 6 files changed, 95 insertions(+), 187 deletions(-) > delete mode 100755 tools/sysprof/collapse.lua > > diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt > index dd7ec6bd..3a919433 100644 > --- a/tools/CMakeLists.txt > +++ b/tools/CMakeLists.txt > diff --git a/tools/sysprof.lua b/tools/sysprof.lua > index 1afab195..be2a0565 100644 > --- a/tools/sysprof.lua > +++ b/tools/sysprof.lua > diff --git a/tools/sysprof/collapse.lua b/tools/sysprof/collapse.lua > deleted file mode 100755 > index 3d83d5ea..00000000 > --- a/tools/sysprof/collapse.lua > +++ /dev/null > diff --git a/tools/sysprof/parse.lua b/tools/sysprof/parse.lua > index 5b52f104..3db36472 100755 > --- a/tools/sysprof/parse.lua > +++ b/tools/sysprof/parse.lua > end > > -local function parse_ffunc(reader, event, _) > +local function parse_ffunc(reader, _) > local ffid = reader:read_uleb128() > - table.insert(event.lua.callchain, 1, { > - type = M.FRAME.FFUNC, > - ffid = ffid, > - }) > + return vmdef.ffnames[ffid] Nice, good changes! > end > > local function parse_lua_callchain(reader, event, symbols) > while true do > local frame_header = reader:read_octet() > - if frame_header == M.FRAME.BOTTOM then > + if frame_header == FRAME.BOTTOM then > break > end > - frame_parsers[frame_header](reader, event, symbols) > + local name = frame_parsers[frame_header](reader, symbols) > + table.insert(event.lua.callchain, 1, {name=name, type=frame_header}) Nit: missed whitespaces around `=`. > end > end > > @@ -100,7 +100,7 @@ local function parse_host_callchain(reader, event, symbols) > @@ -108,10 +108,20 @@ end > --~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~-- > > local function parse_trace_callchain(reader, event, symbols) > - event.lua.trace.traceno = reader:read_uleb128() > - event.lua.trace.addr = reader:read_uleb128() > - event.lua.trace.line = reader:read_uleb128() > - event.lua.trace.gen = symtab.loc(symbols, event.lua.trace).gen > + local loc = { > + traceno = reader:read_uleb128(), > + addr = reader:read_uleb128(), > + line = reader:read_uleb128() OK, this looks fragile. Yes, LuaJIT parser returns bytecode, that execude this in order, but I suggest to rewrite it in more clear way: | local loc = {} | loc.traceno = reader:read_uleb128() | loc.addr = reader:read_uleb128() | loc.line = reader:read_uleb128() > + } > > +local function insert_lua_callchain(chain, lua) > + local ins_cnt = 0 > + local name_lua > + for _, fr in ipairs(lua.callchain) do > + ins_cnt = ins_cnt + 1 > + if fr.type == FRAME.CFUNC then > + -- C function encountered, the next chunk > + -- of frames is located on the C stack. > + break > + end > + name_lua = fr.name > + > + if fr.type == FRAME.LFUNC and lua.trace.traceno ~= nil and > + lua.trace.addr == fr.addr and lua.trace.line == fr.line then > + name_lua = lua.trace.name > + end Minor: I suggest formating like the following: | if fr.type == FRAME.LFUNC | and lua.trace.traceno ~= nil | and lua.trace.addr == fr.addr | and lua.trace.line == fr.line | then | name_lua = lua.trace.name | end or | if | fr.type == FRAME.LFUNC | and lua.trace.traceno ~= nil | and lua.trace.addr == fr.addr | and lua.trace.line == fr.line | then | name_lua = lua.trace.name | end > + > + table.insert(chain, name_lua) > + end > + table.remove(lua.callchain, ins_cnt) > +end > + > +local function merge(event) > + local cc = {} > + > + for _, name_host in ipairs(event.host.callchain) do > + table.insert(cc, name_host) > + if string.match(name_host, '^lua_cpcall') ~= nil then > + -- Any C function is present on both the C and the Lua > + -- stacks. It is more convenient to get its info from the > + -- host stack, since it has information about child frames. > + table.remove(event.lua.callchain) > + end > + > + if string.match(name_host, '^lua_p?call') ~= nil then > + insert_lua_callchain(cc, event.lua) > + end > + > + end > + return cc > +end > + > local function parse_event(reader, events, symbols) > local event = new_event() > > @@ -171,8 +223,10 @@ local function parse_event(reader, events, symbols) > event.lua.vmstate = vmstate > > event_parsers[vmstate](reader, event, symbols) > - > - table.insert(events, event) > + local cc = merge(event) > + local cc_str = table.concat(cc, ';') .. ';' Should we just return cc_str from merge? It will be look like really merged stack. Also, what does cc|cc_str mean?:) > + local cur_cnt = events[cc_str] > + events[cc_str] = (cur_cnt or 0) + 1 > return true > end > > @@ -203,4 +257,5 @@ function M.parse(reader, symbols) > return events > end > > + > return M > diff --git a/tools/utils/avl.lua b/tools/utils/avl.lua > index d5baa534..098f58ec 100644 > --- a/tools/utils/avl.lua > +++ b/tools/utils/avl.lua > diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua > index c26a9e8c..7f6c78f0 100644 > --- a/tools/utils/symtab.lua > +++ b/tools/utils/symtab.lua > -- > 2.40.1 > -- Best regards, Sergey Kaplun