Re: [Tarantool-patches] [PATCH luajit] sysprof: improve parser's memory footprint

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

<snipped>

> diff --git a/tools/sysprof.lua b/tools/sysprof.lua
> index 1afab195..be2a0565 100644
> --- a/tools/sysprof.lua
> +++ b/tools/sysprof.lua

<snipped>

> 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

<snipped>

> 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

<snipped>

>  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
>  

<snipped>

>  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)

<snipped>

> @@ -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()

> +  }

<snipped>

>  
> +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

<snipped>

> 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

<snipped>

> -- 
> 2.40.1
> 

-- 
Best regards,
Sergey Kaplun