Re: [Tarantool-patches] [PATCH luajit 7/7] tools: introduce parsers for sysprof

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

Hi, Maxim!

Thanks for the patch!

Please consider my comments below.

| tools: introduce parsers for sysprof

Typo: s/parsers/parser/

On 08.09.21, Maxim Kokryashkin wrote:
> Since the sysprof's binary output is not human-readable, so there is a
> demand to create a parser. The parser, which this commit provides,
> converts sysprof's event stream into the format that the flamegraph.pl

Minor: It will be nice to add a reference to flamegraph.pl here.

> can process.
> 
> The sysprof parser machinery uses the same symtab module as memprof's
> parser since the format is the same.
> 
> Part of tarantool/tarantool#781

Minor: As far it is the last commit in patchset it should be Resolves
instead.

> ---
>  .gitignore                                    |   1 +
>  .../misclib-sysprof-lapi.test.lua             |   2 +
>  tools/CMakeLists.txt                          |  83 ++++++++
>  tools/luajit-parse-sysprof.in                 |   6 +
>  tools/sysprof.lua                             | 119 +++++++++++
>  tools/sysprof/collapse.lua                    | 110 ++++++++++
>  tools/sysprof/parse.lua                       | 188 ++++++++++++++++++
>  7 files changed, 509 insertions(+)
>  create mode 100644 tools/luajit-parse-sysprof.in

Please make this file executable to make the resultt file executable
too.

>  create mode 100644 tools/sysprof.lua
>  create mode 100755 tools/sysprof/collapse.lua
>  create mode 100755 tools/sysprof/parse.lua

And vice versa -- these files shouldn't be executable.

> 
> diff --git a/.gitignore b/.gitignore
> index 2103a30f..099df060 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -19,4 +19,5 @@ cmake_uninstall.cmake

<snipped>

> diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> index 17d83d1b..b9b3461f 100644
> --- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> @@ -8,6 +8,7 @@ jit.flush()
>  
>  local bufread = require "utils.bufread"
>  local symtab = require "utils.symtab"
> +local sysprof = require "sysprof.parse"
>  
>  local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.sysprofdata.tmp.bin")
>  local BAD_PATH = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%1/sysprofdata.tmp.bin")
> @@ -98,6 +99,7 @@ end
>  
>  local reader = bufread.new(TMP_BINFILE)
>  symtab.parse(reader)
> +sysprof.parse(reader)

I suppose it is better to check some entyties in this profile.

>  
>  os.remove(TMP_BINFILE)
>  
> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index 61830e44..93a2f763 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt
> @@ -87,4 +87,87 @@ else()
>    )
>  endif()
>  
> +
> +

Nit: These empty lines are excess.

> +if(LUAJIT_DISABLE_SYSPROF)
> +  message(STATUS "LuaJIT system profiler support is disabled")
> +else()
> +  # XXX: Can use genex here since the value need to be evaluated
> +  # at the configuration phase. Fortunately, we know the exact
> +  # path where LuaJIT binary is located.
> +  set(LUAJIT_TOOLS_BIN ${LUAJIT_BINARY_DIR}/${LUAJIT_CLI_NAME})
> +  set(LUAJIT_TOOLS_DIR ${CMAKE_CURRENT_SOURCE_DIR})
> +  # XXX: Unfortunately, there is no convenient way to set
> +  # particular permissions to the output file via CMake.
> +  # Furthermore, I even failed to copy the given file to the same
> +  # path to change its permissions. After looking at the docs, I
> +  # realized that the valid solution would be too monstrous for
> +  # such a simple task. As a result I've made the template itself
> +  # executable, so the issue is resolved.
> +  configure_file(luajit-parse-sysprof.in luajit-parse-sysprof @ONLY ESCAPE_QUOTES)
> +

Nit: This empty line is excess.

> +
> +  add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS
> +    luajit-parse-sysprof
> +    sysprof/parse.lua
> +    sysprof/collapse.lua
> +    sysprof.lua
> +    utils/bufread.lua
> +    utils/symtab.lua
> +  )
> +  list(APPEND LUAJIT_TOOLS_DEPS tools-parse-sysprof)

<snipped>

>  add_custom_target(LuaJIT-tools DEPENDS ${LUAJIT_TOOLS_DEPS})
> +

Nit: This new line is excess.

> diff --git a/tools/luajit-parse-sysprof.in b/tools/luajit-parse-sysprof.in
> new file mode 100644
> index 00000000..2be25eb3
> --- /dev/null
> +++ b/tools/luajit-parse-sysprof.in

<snipped>

> diff --git a/tools/sysprof.lua b/tools/sysprof.lua
> new file mode 100644
> index 00000000..e6d8cc34
> --- /dev/null
> +++ b/tools/sysprof.lua
> @@ -0,0 +1,119 @@
> +local bufread = require "utils.bufread"
> +local sysprof = require "sysprof.parse"
> +local symtab = require "utils.symtab"
> +local misc = require "sysprof.collapse"
> +
> +local stdout, stderr = io.stdout, io.stderr
> +local match, gmatch = string.match, string.gmatch
> +
> +local split_by_vmstate = false
> +
> +-- Program options.
> +local opt_map = {}
> +
> +function opt_map.help()
> +  stdout:write [[
> +luajit-parse-sysprof - parser of the profile collected
> +                       with LuaJIT's sysprof.
> +
> +SYNOPSIS
> +
> +luajit-parse-sysprof [options] sysprof.bin
> +
> +Supported options are:
> +
> +  --help                            Show this help and exit
> +  --split                           Split callchains by vmstate
> +]]

Side note: -o, --output option seems usefull for sysprof's parser output
too. It may be done in the separate ticket.

> +  os.exit(0)
> +end
> +
> +function opt_map.split()
> +  split_by_vmstate = true
> +end
> +
> +-- Print error and exit with error status.
> +local function opterror(...)
> +  stderr:write("luajit-parse-sysprof.lua: ERROR: ", ...)
> +  stderr:write("\n")
> +  os.exit(1)
> +end
> +
> +-- Parse single option.
> +local function parseopt(opt, args)
> +  local opt_current = #opt == 1 and "-"..opt or "--"..opt
> +  local f = opt_map[opt]
> +  if not f then
> +    opterror("unrecognized option `", opt_current, "'. Try `--help'.\n")
> +  end
> +  f(args)
> +end
> +
> +-- Parse arguments.
> +local function parseargs(args)
> +  -- Process all option arguments.
> +  args.argn = 1
> +  repeat
> +    local a = args[args.argn]
> +    if not a then
> +      break
> +    end
> +    local lopt, opt = match(a, "^%-(%-?)(.+)")
> +    if not opt then
> +      break
> +    end
> +    args.argn = args.argn + 1
> +    if lopt == "" then
> +      -- Loop through short options.
> +      for o in gmatch(opt, ".") do
> +        parseopt(o, args)
> +      end
> +    else
> +      -- Long option.
> +      parseopt(opt, args)
> +    end
> +  until false
> +
> +  -- Check for proper number of arguments.
> +  local nargs = #args - args.argn + 1
> +  if nargs ~= 1 then
> +    opt_map.help()
> +  end
> +
> +  -- Translate a single input file.
> +  -- TODO: Handle multiple files?
> +  return args[args.argn]
> +end

Minor: This logic with parse options is pretty the same for sysprof and
memprof (only options and callbacks are different). So, we can move this
part to the separate module (maybe within the separate commit).

> +
> +local function traverse_calltree(node, prefix)
> +  if node.is_leaf then
> +    print(prefix..' '..node.count)
> +  end
> +
> +  local sep_prefix = #prefix == 0 and prefix or prefix..';'
> +
> +  for name,child in pairs(node.children) do
> +    traverse_calltree(child, sep_prefix..name)
> +  end
> +end
> +
> +local function dump(inputfile)
> +  local reader = bufread.new(inputfile)
> +
> +  local symbols = symtab.parse(reader)
> +
> +  local events = sysprof.parse(reader)
> +  local calltree = misc.collapse(events, symbols, split_by_vmstate)
> +
> +  traverse_calltree(calltree, '')
> +
> +  os.exit(0)
> +end
> +
> +-- FIXME: this script should be application-independent.
> +local args = {...}
> +if #args == 1 and args[1] == "sysprof" then
> +  return dump
> +else
> +  dump(parseargs(args))
> +end
> diff --git a/tools/sysprof/collapse.lua b/tools/sysprof/collapse.lua
> new file mode 100755
> index 00000000..2d684c95
> --- /dev/null
> +++ b/tools/sysprof/collapse.lua
> @@ -0,0 +1,110 @@

Side note: I don't look this part carefully as far as we assume offline
to rework it.

> +local parse = require "sysprof.parse"
> +local vmdef = require "jit.vmdef"
> +local symtab = require "utils.symtab"
> +
> +local VMST_NAMES = {
> +  [parse.VMST.INTERP] = "VMST_INTERP",
> +  [parse.VMST.LFUNC]  = "VMST_LFUNC",
> +  [parse.VMST.FFUNC]  = "VMST_FFUNC",
> +  [parse.VMST.CFUNC]  = "VMST_CFUNC",
> +  [parse.VMST.GC]     = "VMST_GC",
> +  [parse.VMST.EXIT]   = "VMST_EXIT",
> +  [parse.VMST.RECORD] = "VMST_RECORD",
> +  [parse.VMST.OPT]    = "VMST_OPT",
> +  [parse.VMST.ASM]    = "VMST_ASM",
> +  [parse.VMST.TRACE]  = "VMST_TRACE",
> +}
> +
> +local M = {}
> +
> +local function new_node(name, is_leaf)
> +  return {
> +    name = name,
> +    count = 0,
> +    is_leaf = is_leaf,
> +    children = {}
> +  }
> +end
> +
> +-- insert new child into a node (or increase counter in existing one)

Nit: The sentence must start with a capital letter and end with a dot.
Here and below.

> +local function insert(name, node, is_leaf)
> +  if node.children[name] == nil then
> +    node.children[name] = new_node(name, is_leaf)
> +  end
> +
> +  local child = node.children[name]
> +  child.count = child.count + 1
> +
> +  return child
> +end
> +
> +local function insert_lua_callchain(chain, lua, symbols)
> +  for _,fr in pairs(lua.callchain) do

Typo: s/_,fr/_, fr/

> +    local name_lua
> +
> +    if fr.type == parse.FRAME.FFUNC then
> +      name_lua = vmdef.ffnames[fr.ffid]
> +    else
> +      name_lua = symtab.demangle(symbols, {
> +        addr = fr.addr,
> +        line = fr.line
> +      })
> +      if lua.trace.id ~= nil and lua.trace.addr == fr.addr and
> +          lua.trace.line == fr.line then
> +        name_lua = 'TRACE_'..tostring(lua.trace.id)..'_'..name_lua
> +      end
> +    end
> +
> +    table.insert(chain, { name = name_lua })
> +  end
> +end
> +
> +-- merge lua and host callchains into one callchain representing
> +-- transfer of control
> +local function merge(event, symbols, sep_vmst)

Side note: This part should be reworked to show HOST->LUA->HOST->LUA
callchains correctly as we discussed offline. The host stack is always
on top, the Lua stack is started after BC_* (simple waterline).

> +  local cc = {}
> +  local lua_inserted = false
> +
> +  for _,h_fr in pairs(event.host.callchain) do
> +    local name_host = symtab.demangle(symbols, { addr = h_fr.addr })
> +
> +    -- We assume that usually the transfer of control
> +    -- looks like:
> +    --    HOST -> LUA -> HOST
> +    -- so for now, lua callchain starts from lua_pcall() call
> +    if name_host == 'lua_pcall' then
> +      insert_lua_callchain(cc, event.lua, symbols)
> +      lua_inserted = true
> +    end
> +
> +    table.insert(cc, { name = name_host })
> +  end
> +
> +  if lua_inserted == false then

Minor: `not lua_inserted` looks more readable to me.
Feel free to ignore.

> +    insert_lua_callchain(cc, event.lua, symbols)
> +  end
> +
> +  if sep_vmst == true then

Minor: `if sep_vmst then` looks more readable to me.
Feel free to ignore.

> +    table.insert(cc, { name = VMST_NAMES[event.lua.vmstate] })
> +  end
> +
> +  return cc
> +end
> +
> +-- Collapse all the events into call tree
> +function M.collapse(events, symbols, sep_vmst)
> +  local root = new_node('root', false)
> +
> +  for _,ev in pairs(events) do
> +    local callchain = merge(ev, symbols, sep_vmst)
> +    local curr_node = root
> +    for i=#callchain,1,-1 do

Typo: s/,/, /g
Typo: s/=/ = /

> +      curr_node = insert(callchain[i].name, curr_node, false)
> +    end
> +    insert('', curr_node, true)
> +  end
> +
> +  return root
> +end
> +
> +return M
> diff --git a/tools/sysprof/parse.lua b/tools/sysprof/parse.lua
> new file mode 100755
> index 00000000..766b0c99
> --- /dev/null
> +++ b/tools/sysprof/parse.lua
> @@ -0,0 +1,188 @@
> +-- Parser of LuaJIT's sysprof binary stream.
> +-- The format spec can be found in <src/lj_sysprof.h>.
> +
> +local string_format = string.format
> +
> +local LJP_MAGIC = "ljp"
> +local LJP_CURRENT_VERSION = 1
> +
> +local M = {}
> +
> +M.VMST = {
> +  INTERP = 0,
> +  LFUNC  = 1,
> +  FFUNC  = 2,
> +  CFUNC  = 3,
> +  GC     = 4,
> +  EXIT   = 5,
> +  RECORD = 6,
> +  OPT    = 7,
> +  ASM    = 8,
> +  TRACE  = 9,
> +}
> +
> +
> +M.FRAME = {
> +  LFUNC  = 1,
> +  CFUNC  = 2,
> +  FFUNC  = 3,
> +  BOTTOM = 0x80
> +}
> +
> +local STREAM_END = 0x80
> +
> +local function new_event()
> +  return {
> +    lua = {
> +      vmstate = 0,
> +      callchain = {},
> +      trace = {

Minor: looks like trace should be separated as unique case as Lua or
host, doesn't it?

> +        id = nil,
> +        addr = 0,
> +        line = 0
> +      }

Minor: It is better to use "," in the last entries too: it is easier to
add the other entries, if you want.
Here and below.

> +    },
> +    host = {
> +      callchain = {}
> +    }
> +  }
> +end
> +
> +local function parse_lfunc(reader, event)
> +  local addr = reader:read_uleb128()
> +  local line = reader:read_uleb128()
> +  table.insert(event.lua.callchain, {
> +    type = M.FRAME.LFUNC,
> +    addr = addr,
> +    line = line
> +  })
> +end
> +
> +local function parse_ffunc(reader, event)
> +  local ffid = reader:read_uleb128()
> +  table.insert(event.lua.callchain, {
> +    type = M.FRAME.FFUNC,
> +    ffid = ffid,
> +  })
> +end
> +
> +local function parse_cfunc(reader, event)
> +  local addr = reader:read_uleb128()
> +  table.insert(event.lua.callchain, {
> +    type = M.FRAME.CFUNC,
> +    addr = addr
> +  })
> +end
> +
> +local frame_parsers = {
> +  [M.FRAME.LFUNC] = parse_lfunc,
> +  [M.FRAME.FFUNC] = parse_ffunc,
> +  [M.FRAME.CFUNC] = parse_cfunc
> +}
> +
> +local function parse_lua_callchain(reader, event)
> +  while true do
> +    local frame_header = reader:read_octet()
> +    if frame_header == M.FRAME.BOTTOM then
> +      break
> +    end
> +    frame_parsers[frame_header](reader, event)
> +  end
> +end
> +
> +--~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~--

Side note: what does this curves line separate, only
parse_host_callchain? Also, IMO it is pretier to use `-` instead.
Feel free to ignore.

> +
> +local function parse_host_callchain(reader, event)
> +  local addr = reader:read_uleb128()
> +
> +  while addr ~= 0 do

Side note: NULL byte is always written for last host frame?

> +    table.insert(event.host.callchain, {
> +      addr = addr
> +    })
> +    addr = reader:read_uleb128()
> +  end
> +end
> +
> +--~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~--
> +
> +local function parse_trace_callchain(reader, event)
> +  event.lua.trace.id   = reader:read_uleb128()
> +  event.lua.trace.addr = reader:read_uleb128()
> +  event.lua.trace.line = reader:read_uleb128()
> +end
> +
> +--~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~--
> +
> +local function parse_host_only(reader, event)
> +  parse_host_callchain(reader, event)
> +end
> +
> +local function parse_lua_host(reader, event)
> +  parse_lua_callchain(reader, event)
> +  parse_host_callchain(reader, event)
> +end
> +
> +local function parse_trace(reader, event)
> +  parse_trace_callchain(reader, event)
> +  -- parse_lua_callchain(reader, event)

Minor: Looks like this comment should be deleted.

> +end
> +
> +local event_parsers = {
> +  [M.VMST.INTERP] = parse_host_only,
> +  [M.VMST.LFUNC]  = parse_lua_host,
> +  [M.VMST.FFUNC]  = parse_lua_host,
> +  [M.VMST.CFUNC]  = parse_lua_host,
> +  [M.VMST.GC]     = parse_host_only,
> +  [M.VMST.EXIT]   = parse_host_only,
> +  [M.VMST.RECORD] = parse_host_only,
> +  [M.VMST.OPT]    = parse_host_only,
> +  [M.VMST.ASM]    = parse_host_only,
> +  [M.VMST.TRACE]  = parse_trace
> +}
> +
> +local function parse_event(reader, events)
> +  local event = new_event()
> +
> +  local vmstate = reader:read_octet()
> +  if vmstate == STREAM_END then
> +    -- TODO: samples & overruns
> +    return false
> +  end
> +
> +  assert(0 <= vmstate and vmstate <= 9, "Vmstate "..vmstate.." is not valid")

Minor: For what 0 and 9 stands for?

> +  event.lua.vmstate = vmstate
> +
> +  event_parsers[vmstate](reader, event)
> +
> +  table.insert(events, event)
> +  return true
> +end
> +
> +function M.parse(reader)

<snipped>

> +end
> +
> +return M
> -- 
> 2.33.0
> 

-- 
Best regards,
Sergey Kaplun