[Tarantool-patches] [PATCH luajit 0/4] profilers: refactor parsers

[Tarantool-patches] [PATCH luajit 0/4] profilers: refactor parsers

[Maxim Kokryashkin via Tarantool-patches]

This patchset contains various imporvements for profile parsers.
This set of changes is only the first part of what's coming after
the patch with ptrace testing. All of the serious bug fixes will
be included in the second part.

Branch: https://github.com/tarantool/luajit/tree/fckxorg/profile-parsers-refactoring
PR: https://github.com/tarantool/tarantool/pull/9438
Issues:
https://github.com/tarantool/tarantool/issues/5994
https://github.com/tarantool/tarantool/issues/9217

Maxim Kokryashkin (4):
  cmake: properly handle the memprof/process.lua
  memprof: refactor `heap_chunk` data structure
  memprof: introduce the `--human-readable` option
  profilers: print user-friendly errors

 .../gh-5688-tool-cli-flag.test.lua            |  4 +-
 .../gh-5994-memprof-human-readable.test.lua   | 72 +++++++++++++++++
 ...17-profile-parsers-error-handling.test.lua | 79 +++++++++++++++++++
 tools/CMakeLists.txt                          |  2 +
 tools/memprof.lua                             | 68 ++++++++++++----
 tools/memprof/humanize.lua                    | 46 ++++++++---
 tools/memprof/parse.lua                       | 22 ++++--
 tools/sysprof.lua                             | 42 ++++++++--
 tools/sysprof/parse.lua                       |  2 +-
 tools/utils/symtab.lua                        |  2 +-
 10 files changed, 297 insertions(+), 42 deletions(-)
 create mode 100644 test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
 create mode 100644 test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua

--
2.43.0

[Tarantool-patches] [PATCH luajit 1/4] cmake: properly handle the memprof/process.lua

[Maxim Kokryashkin via Tarantool-patches]

Prior to this patch, memprof/process.lua wasn't added to the
dependency list as a part of the memprof parser sources. Also,
it wasn't installed into the system along with other memprof
sources, which breaks the profile parser.

Part of tarantool/tarantool#5994
---
 tools/CMakeLists.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
index b951f767..abf68423 100644
--- a/tools/CMakeLists.txt
+++ b/tools/CMakeLists.txt
@@ -14,6 +14,7 @@ else()
   add_custom_target(tools-parse-memprof EXCLUDE_FROM_ALL DEPENDS
     memprof/humanize.lua
     memprof/parse.lua
+    memprof/process.lua
     memprof.lua
     utils/avl.lua
     utils/bufread.lua
@@ -24,6 +25,7 @@ else()
   install(FILES
       ${CMAKE_CURRENT_SOURCE_DIR}/memprof/humanize.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/memprof/parse.lua
+      ${CMAKE_CURRENT_SOURCE_DIR}/memprof/process.lua
     DESTINATION ${LUAJIT_DATAROOTDIR}/memprof
     PERMISSIONS
       OWNER_READ OWNER_WRITE
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 2/4] memprof: refactor `heap_chunk` data structure

[Maxim Kokryashkin via Tarantool-patches]

The memprof parser used to have the `heap_chunk` data structure
using numeric indices for its values, which is hardly readable
and maintainable. This patch replaces these numeric indices with
corresponding string keys.

Part of tarantool/tarantool#5994
---
 tools/memprof/parse.lua | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
index 42a601ef..cc66a23e 100644
--- a/tools/memprof/parse.lua
+++ b/tools/memprof/parse.lua
@@ -45,22 +45,30 @@ local function new_event(loc)
   }
 end
 
+local function new_heap_chunk(size, id, loc)
+  return {
+    size = size,
+    id = id,
+    loc = loc,
+  }
+end
+
 local function link_to_previous(heap_chunk, e, nsize)
   -- Memory at this chunk was allocated before we start tracking.
   if heap_chunk then
     -- Save Lua code location (line) by address (id).
-    if not e.primary[heap_chunk[2]] then
-      e.primary[heap_chunk[2]] = {
-        loc = heap_chunk[3],
+    if not e.primary[heap_chunk.id] then
+      e.primary[heap_chunk.id] = {
+        loc = heap_chunk.loc,
         allocated = 0,
         freed = 0,
         count = 0,
       }
     end
     -- Save information about delta for memory heap.
-    local location_data = e.primary[heap_chunk[2]]
+    local location_data = e.primary[heap_chunk.id]
     location_data.allocated = location_data.allocated + nsize
-    location_data.freed = location_data.freed + heap_chunk[1]
+    location_data.freed = location_data.freed + heap_chunk.size
     location_data.count = location_data.count + 1
   end
 end
@@ -95,7 +103,7 @@ local function parse_alloc(reader, asource, events, heap, symbols)
   e.num = e.num + 1
   e.alloc = e.alloc + nsize
 
-  heap[naddr] = {nsize, id, loc}
+  heap[naddr] = new_heap_chunk(nsize, id, loc)
 end
 
 local function parse_realloc(reader, asource, events, heap, symbols)
@@ -117,7 +125,7 @@ local function parse_realloc(reader, asource, events, heap, symbols)
   link_to_previous(heap[oaddr], e, nsize)
 
   heap[oaddr] = nil
-  heap[naddr] = {nsize, id, loc}
+  heap[naddr] = new_heap_chunk(nsize, id, loc)
 end
 
 local function parse_free(reader, asource, events, heap, symbols)
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 3/4] memprof: introduce the `--human-readable` option

[Maxim Kokryashkin via Tarantool-patches]

Prior to this patch, memprof could report only the raw
amount of bytes, which is hard to read. This patch adds the
`--human-readable` CLI option to the memprof parser, so the
memory is reported in KiB, MiB, or GiB, depending on what's
the biggest reasonable unit.

This patch also refactors the options mechanism for parser,
so all of the options are passed into parser's modules as a
single config table instead of being handled individually.

Part of tarantool/tarantool#5994
---
 .../gh-5994-memprof-human-readable.test.lua   | 72 +++++++++++++++++++
 tools/memprof.lua                             | 21 ++++--
 tools/memprof/humanize.lua                    | 46 +++++++++---
 3 files changed, 122 insertions(+), 17 deletions(-)
 create mode 100644 test/tarantool-tests/gh-5994-memprof-human-readable.test.lua

diff --git a/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
new file mode 100644
index 00000000..0a579cec
--- /dev/null
+++ b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
@@ -0,0 +1,72 @@
+local tap = require('tap')
+local test = tap.test('gh-5994-memprof-human-readable'):skipcond({
+  ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and
+                                                      jit.arch ~= 'x64',
+  ['Profile tools are implemented for Linux only'] = jit.os ~= 'Linux',
+  -- XXX: Tarantool integration is required to run this test properly.
+  -- luacheck: no global
+  ['No profile tools CLI option integration'] = _TARANTOOL,
+})
+test:plan(5)
+
+local utils = require('utils')
+local TMP_BINFILE_MEMPROF = utils.tools.profilename('memprofdata.tmp.bin')
+local PARSE_CMD = utils.exec.luacmd(arg) .. ' -tm '
+
+local function generate_output(bytes)
+  local res, err = misc.memprof.start(TMP_BINFILE_MEMPROF)
+  -- Should start successfully.
+  assert(res, err)
+
+  -- luacheck: no unused
+  local _ = string.rep('_', bytes)
+
+  res, err = misc.memprof.stop()
+  -- Should stop successfully.
+  assert(res, err)
+end
+
+local TEST_SET = {
+  {
+    bytes = 2049,
+    match = '%dB',
+    hr = false,
+    name = 'non-human-readable mode is correct'
+  },
+  {
+    bytes = 100,
+    match = '%dB',
+    hr = true,
+    name = 'human-readable mode: bytes'
+  },
+  {
+    bytes = 2560,
+    match = '%d+%.%d%dKiB',
+    hr = true,
+    name = 'human-readable mode: float'
+  },
+  {
+    bytes = 2048,
+    match = '%dKiB',
+    hr = true,
+    name = 'human-readable mode: KiB'
+  },
+  {
+    bytes = 1024 * 1024,
+    match = '%dMiB',
+    hr = true,
+    name = 'human-readable mode: MiB'
+  },
+  -- XXX: The test case for GiB is not implemented because it is
+  -- OOM-prone for non-GC64 builds.
+}
+
+for _, params in ipairs(TEST_SET) do
+  generate_output(params.bytes)
+  local cmd = PARSE_CMD .. (params.hr and ' --human-readable ' or '')
+  local output = io.popen(cmd .. TMP_BINFILE_MEMPROF):read('*all')
+  test:like(output, params.match, params.name)
+end
+
+os.remove(TMP_BINFILE_MEMPROF)
+test:done(true)
diff --git a/tools/memprof.lua b/tools/memprof.lua
index e23bbf24..acadbc17 100644
--- a/tools/memprof.lua
+++ b/tools/memprof.lua
@@ -22,6 +22,12 @@ local match, gmatch = string.match, string.gmatch
 -- Program options.
 local opt_map = {}
 
+-- Default config for the memprof parser.
+local config = {
+  leak_only = false,
+  human_readable = false,
+}
+
 function opt_map.help()
   stdout:write [[
 luajit-parse-memprof - parser of the memory usage profile collected
@@ -39,9 +45,12 @@ Supported options are:
   os.exit(0)
 end
 
-local leak_only = false
 opt_map["leak-only"] = function()
-  leak_only = true
+  config.leak_only = true
+end
+
+opt_map["human-readable"] = function()
+  config.human_readable = true
 end
 
 -- Print error and exit with error status.
@@ -101,11 +110,11 @@ local function dump(inputfile)
   local reader = bufread.new(inputfile)
   local symbols = symtab.parse(reader)
   local events = memprof.parse(reader, symbols)
-  if not leak_only then
-    view.profile_info(events, symbols)
+  if not config.leak_only then
+    view.profile_info(events, config)
   end
-  local dheap = process.form_heap_delta(events, symbols)
-  view.leak_info(dheap)
+  local dheap = process.form_heap_delta(events)
+  view.leak_info(dheap, config)
   view.aliases(symbols)
   -- XXX: The second argument is required to properly close Lua
   -- universe (i.e. invoke <lua_close> before exiting).
diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
index 5b289ce3..a5fdabf7 100644
--- a/tools/memprof/humanize.lua
+++ b/tools/memprof/humanize.lua
@@ -5,7 +5,29 @@
 
 local M = {}
 
-function M.render(events)
+local function human_readable_bytes(bytes)
+  local units = {"B", "KiB", "MiB", "GiB"}
+  local i = 1
+
+  while bytes >= 1024 and i < #units do
+    bytes = bytes / 1024
+    i = i + 1
+  end
+  local is_int = math.floor(bytes) == bytes
+  local fmt = is_int and "%d%s" or "%.2f%s"
+
+  return string.format(fmt, bytes, units[i])
+end
+
+local function format_bytes(bytes, config)
+  if config.human_readable then
+    return human_readable_bytes(bytes)
+  else
+    return string.format('%dB', bytes)
+  end
+end
+
+function M.render(events, config)
   local ids = {}
 
   for id, _ in pairs(events) do
@@ -18,11 +40,11 @@ function M.render(events)
 
   for i = 1, #ids do
     local event = events[ids[i]]
-    print(string.format("%s: %d events\t+%d bytes\t-%d bytes",
+    print(string.format("%s: %d events\t+%s\t-%s",
       event.loc,
       event.num,
-      event.alloc,
-      event.free
+      format_bytes(event.alloc, config),
+      format_bytes(event.free, config)
     ))
 
     local prim_loc = {}
@@ -40,21 +62,21 @@ function M.render(events)
   end
 end
 
-function M.profile_info(events)
+function M.profile_info(events, config)
   print("ALLOCATIONS")
-  M.render(events.alloc)
+  M.render(events.alloc, config)
   print("")
 
   print("REALLOCATIONS")
-  M.render(events.realloc)
+  M.render(events.realloc, config)
   print("")
 
   print("DEALLOCATIONS")
-  M.render(events.free)
+  M.render(events.free, config)
   print("")
 end
 
-function M.leak_info(dheap)
+function M.leak_info(dheap, config)
   local leaks = {}
   for line, info in pairs(dheap) do
     -- Report "INTERNAL" events inconsistencies for profiling
@@ -71,8 +93,10 @@ function M.leak_info(dheap)
   print("HEAP SUMMARY:")
   for _, l in pairs(leaks) do
     print(string.format(
-      "%s holds %d bytes: %d allocs, %d frees",
-      l.line, l.dbytes, dheap[l.line].nalloc,
+      "%s holds %s: %d allocs, %d frees",
+      l.line,
+      format_bytes(l.dbytes, config),
+      dheap[l.line].nalloc,
       dheap[l.line].nfree
     ))
   end
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 4/4] profilers: print user-friendly errors

[Maxim Kokryashkin via Tarantool-patches]

Prior to this patch, there was no error-checking in profilers,
which resulted in raw Lua errors being reported in cases of
non-existing paths or corrupt file structures. This patch adds
error handling, so all parsing errors are now reported in a
user-friendly manner.

Event parsing is now moved into a separate profiler-agnostic
module.

Tool CLI flag tests are adapted correspondingly to error message
changes.

Resolves tarantool/tarantool#9217
Part of tarantool/tarantool#5994
---
 .../gh-5688-tool-cli-flag.test.lua            |  4 +-
 ...17-profile-parsers-error-handling.test.lua | 79 +++++++++++++++++++
 tools/memprof.lua                             | 47 +++++++++--
 tools/sysprof.lua                             | 42 ++++++++--
 tools/sysprof/parse.lua                       |  2 +-
 tools/utils/symtab.lua                        |  2 +-
 6 files changed, 158 insertions(+), 18 deletions(-)
 create mode 100644 test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua

diff --git a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
index 75293f11..ec958031 100644
--- a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
+++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
@@ -42,7 +42,7 @@ local SMOKE_CMD_SET = {
 local MEMPROF_CMD_SET = {
   {
     cmd = MEMPROF_PARSER .. BAD_PATH,
-    like = 'fopen, errno: 2',
+    like = 'Failed to open',
   },
   {
     cmd = MEMPROF_PARSER .. TMP_BINFILE_MEMPROF,
@@ -61,7 +61,7 @@ local MEMPROF_CMD_SET = {
 local SYSPROF_CMD_SET = {
   {
     cmd = SYSPROF_PARSER .. BAD_PATH,
-    like = 'fopen, errno: 2',
+    like = 'Failed to open',
   },
   {
     cmd = SYSPROF_PARSER .. TMP_BINFILE_SYSPROF,
diff --git a/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
new file mode 100644
index 00000000..9a818086
--- /dev/null
+++ b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
@@ -0,0 +1,79 @@
+local tap = require('tap')
+local test = tap.test('gh-9217-profile-parsers-error-handling'):skipcond({
+  ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and
+                                                      jit.arch ~= 'x64',
+  ['Profile tools are implemented for Linux only'] = jit.os ~= 'Linux',
+  -- XXX: Tarantool integration is required to run this test properly.
+  -- luacheck: no global
+  ['No profile tools CLI option integration'] = _TARANTOOL,
+})
+
+test:plan(6)
+
+jit.off()
+jit.flush()
+
+local table_new = require('table.new')
+local utils = require('utils')
+
+local BAD_PATH = utils.tools.profilename('bad-path-tmp.bin')
+local NON_PROFILE_DATA = utils.tools.profilename('not-profile-data.tmp.bin')
+local CORRUPT_PROFILE = utils.tools.profilename('profdata.tmp.bin')
+
+local EXECUTABLE = utils.exec.luacmd(arg)
+local PARSERS = {
+  memprof = EXECUTABLE .. ' -tm ',
+  sysprof = EXECUTABLE .. ' -ts ',
+}
+local REDIRECT_OUTPUT = ' 2>&1'
+
+local TABLE_SIZE = 20
+
+local TEST_CASES = {
+  [BAD_PATH] = 'Failed to open',
+  [NON_PROFILE_DATA] = 'Failed to parse symtab from',
+  [CORRUPT_PROFILE] = 'Failed to parse profile data from',
+}
+
+local function generate_non_profile_data(path)
+  local file = io.open(path, 'w')
+  file:write('data')
+  file:close()
+end
+
+local function generate_corrupt_profile(path)
+  local res, err = misc.memprof.start(path)
+  -- Should start successfully.
+  assert(res, err)
+
+  local _ = table_new(TABLE_SIZE, 0)
+   _ = nil
+  collectgarbage()
+
+  res, err = misc.memprof.stop()
+  -- Should stop successfully.
+  assert(res, err)
+
+  local file = io.open(path, 'r')
+  local content = file:read('*all')
+  file:close()
+  local index = string.find(content, 'ljm')
+
+  file = io.open(path, 'w')
+  file:write(string.sub(content, 1, index - 1))
+  file:close()
+end
+
+generate_non_profile_data(NON_PROFILE_DATA)
+generate_corrupt_profile(CORRUPT_PROFILE)
+
+for path, err_msg in pairs(TEST_CASES) do
+  for profiler, parser in pairs(PARSERS) do
+    local output = io.popen(parser .. path .. REDIRECT_OUTPUT):read('*all')
+    test:like(output, err_msg, string.format('%s: %s', profiler, err_msg))
+  end
+end
+
+os.remove(NON_PROFILE_DATA)
+os.remove(CORRUPT_PROFILE)
+test:done(true)
diff --git a/tools/memprof.lua b/tools/memprof.lua
index acadbc17..a04608b8 100644
--- a/tools/memprof.lua
+++ b/tools/memprof.lua
@@ -10,11 +10,11 @@
 -- Major portions taken verbatim or adapted from the LuaVela.
 -- Copyright (C) 2015-2019 IPONWEB Ltd.
 
-local bufread = require "utils.bufread"
-local memprof = require "memprof.parse"
-local process = require "memprof.process"
-local symtab = require "utils.symtab"
-local view = require "memprof.humanize"
+local bufread = require('utils.bufread')
+local symtab = require('utils.symtab')
+local memprof = require('memprof.parse')
+local process = require('memprof.process')
+local view = require('memprof.humanize')
 
 local stdout, stderr = io.stdout, io.stderr
 local match, gmatch = string.match, string.gmatch
@@ -106,10 +106,41 @@ local function parseargs(args)
   return args[args.argn]
 end
 
+local function make_error_handler(inputfile, fmt)
+  return function()
+    io.stderr:write(string.format(fmt, inputfile))
+    os.exit(1, true)
+  end
+end
+
+local function safe_event_reader(inputfile)
+  local _, reader = xpcall(
+    bufread.new,
+    make_error_handler(inputfile, 'Failed to open %s.'),
+    inputfile
+  )
+
+  local _, symbols = xpcall(
+    symtab.parse,
+    make_error_handler(inputfile, 'Failed to parse symtab from %s.'),
+    reader
+  )
+
+  local _, events = xpcall(
+    memprof.parse,
+    make_error_handler(inputfile, 'Failed to parse profile data from %s.'),
+    reader,
+    symbols
+  )
+  return events, symbols
+end
+
 local function dump(inputfile)
-  local reader = bufread.new(inputfile)
-  local symbols = symtab.parse(reader)
-  local events = memprof.parse(reader, symbols)
+  -- XXX: This function exits with a non-zero exit code and
+  -- prints an error message if it encounters any failure during
+  -- the process of parsing.
+  local events, symbols = safe_event_reader(inputfile)
+
   if not config.leak_only then
     view.profile_info(events, config)
   end
diff --git a/tools/sysprof.lua b/tools/sysprof.lua
index 8449b23f..d2efcd7f 100644
--- a/tools/sysprof.lua
+++ b/tools/sysprof.lua
@@ -1,6 +1,6 @@
-local bufread = require "utils.bufread"
-local sysprof = require "sysprof.parse"
-local symtab = require "utils.symtab"
+local bufread = require('utils.bufread')
+local symtab = require('utils.symtab')
+local sysprof = require('sysprof.parse')
 
 local stdout, stderr = io.stdout, io.stderr
 local match, gmatch = string.match, string.gmatch
@@ -77,10 +77,40 @@ local function parseargs(args)
   return args[args.argn]
 end
 
+local function make_error_handler(inputfile, fmt)
+  return function()
+    io.stderr:write(string.format(fmt, inputfile))
+    os.exit(1, true)
+  end
+end
+
+local function safe_event_reader(inputfile)
+  local _, reader = xpcall(
+    bufread.new,
+    make_error_handler(inputfile, 'Failed to open %s.'),
+    inputfile
+  )
+
+  local _, symbols = xpcall(
+    symtab.parse,
+    make_error_handler(inputfile, 'Failed to parse symtab from %s.'),
+    reader
+  )
+
+  local _, events = xpcall(
+    sysprof.parse,
+    make_error_handler(inputfile, 'Failed to parse profile data from %s.'),
+    reader,
+    symbols
+  )
+  return events, symbols
+end
+
 local function dump(inputfile)
-  local reader = bufread.new(inputfile)
-  local symbols = symtab.parse(reader)
-  local events = sysprof.parse(reader, symbols)
+  -- XXX: This function exits with a non-zero exit code and
+  -- prints an error message if it encounters any failure during
+  -- the process of parsing.
+  local events = safe_event_reader(inputfile)
 
   for stack, count in pairs(events) do
     print(stack, count)
diff --git a/tools/sysprof/parse.lua b/tools/sysprof/parse.lua
index 64c4a455..749f70db 100755
--- a/tools/sysprof/parse.lua
+++ b/tools/sysprof/parse.lua
@@ -237,7 +237,7 @@ function M.parse(reader, symbols)
   local _ = reader:read_octets(3)
 
   if magic ~= LJP_MAGIC then
-    error("Bad LJP format prologue: "..magic)
+    error("Bad LJP format prologue: " .. tostring(magic))
   end
 
   if string.byte(version) ~= LJP_CURRENT_VERSION then
diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua
index 0c878e7a..c4aefef7 100644
--- a/tools/utils/symtab.lua
+++ b/tools/utils/symtab.lua
@@ -95,7 +95,7 @@ function M.parse(reader)
   local _ = reader:read_octets(3)
 
   if magic ~= LJS_MAGIC then
-    error("Bad LJS format prologue: "..magic)
+    error("Bad LJS format prologue: " .. tostring(magic))
   end
 
   if string.byte(version) ~= LJS_CURRENT_VERSION then
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit 1/4] cmake: properly handle the memprof/process.lua

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
Please consider my comment below.

On 04.12.23, Maxim Kokryashkin wrote:
> Prior to this patch, memprof/process.lua wasn't added to the
> dependency list as a part of the memprof parser sources. Also,
> it wasn't installed into the system along with other memprof
> sources, which breaks the profile parser.
> 
> Part of tarantool/tarantool#5994
> ---
>  tools/CMakeLists.txt | 2 ++

I suppose this patch should also add <memprof/process.lua> to the
`FILES_MEMPROFLIB=` in the <Makefile.original>.

Also, we should add sysprof libraries to the original Makefile.

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/4] memprof: refactor `heap_chunk` data structure

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 3/4] memprof: introduce the `--human-readable` option

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
Generally LGTM, after fixing my nits below.

On 04.12.23, Maxim Kokryashkin wrote:
> Prior to this patch, memprof could report only the raw
> amount of bytes, which is hard to read. This patch adds the
> `--human-readable` CLI option to the memprof parser, so the
> memory is reported in KiB, MiB, or GiB, depending on what's
> the biggest reasonable unit.
> 
> This patch also refactors the options mechanism for parser,
> so all of the options are passed into parser's modules as a
> single config table instead of being handled individually.
> 
> Part of tarantool/tarantool#5994
> ---
>  .../gh-5994-memprof-human-readable.test.lua   | 72 +++++++++++++++++++
>  tools/memprof.lua                             | 21 ++++--
>  tools/memprof/humanize.lua                    | 46 +++++++++---
>  3 files changed, 122 insertions(+), 17 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
> 
> diff --git a/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
> new file mode 100644
> index 00000000..0a579cec
> --- /dev/null
> +++ b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
> @@ -0,0 +1,72 @@
> +local tap = require('tap')
> +local test = tap.test('gh-5994-memprof-human-readable'):skipcond({
> +  ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and
> +                                                      jit.arch ~= 'x64',
> +  ['Profile tools are implemented for Linux only'] = jit.os ~= 'Linux',
> +  -- XXX: Tarantool integration is required to run this test properly.
> +  -- luacheck: no global
> +  ['No profile tools CLI option integration'] = _TARANTOOL,
> +})
> +test:plan(5)

We can use `#TEST_SET` here instead of `5`.

> +

<snipped

> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index e23bbf24..acadbc17 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua

<snipped>

> -local leak_only = false
>  opt_map["leak-only"] = function()
> -  leak_only = true
> +  config.leak_only = true
> +end
> +
> +opt_map["human-readable"] = function()
> +  config.human_readable = true
>  end

It's good to add this option in the help ouput, too.

>  
>  -- Print error and exit with error status.
> @@ -101,11 +110,11 @@ local function dump(inputfile)
>    local reader = bufread.new(inputfile)
>    local symbols = symtab.parse(reader)
>    local events = memprof.parse(reader, symbols)
> -  if not leak_only then
> -    view.profile_info(events, symbols)
> +  if not config.leak_only then
> +    view.profile_info(events, config)
>    end
> -  local dheap = process.form_heap_delta(events, symbols)

I understand that this is minor change, but it's better to add this
clenup via the separate commit.

> -  view.leak_info(dheap)
> +  local dheap = process.form_heap_delta(events)
> +  view.leak_info(dheap, config)
>    view.aliases(symbols)
>    -- XXX: The second argument is required to properly close Lua
>    -- universe (i.e. invoke <lua_close> before exiting).
> diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
> index 5b289ce3..a5fdabf7 100644
> --- a/tools/memprof/humanize.lua
> +++ b/tools/memprof/humanize.lua
> @@ -5,7 +5,29 @@
>  
>  local M = {}
>  
> -function M.render(events)
> +local function human_readable_bytes(bytes)
> +  local units = {"B", "KiB", "MiB", "GiB"}
> +  local i = 1

Minor: maybe it's better to use `magnitude` (or something like) instead
of just `i`.

> +
> +  while bytes >= 1024 and i < #units do
> +    bytes = bytes / 1024
> +    i = i + 1
> +  end
> +  local is_int = math.floor(bytes) == bytes
> +  local fmt = is_int and "%d%s" or "%.2f%s"
> +
> +  return string.format(fmt, bytes, units[i])
> +end
> +
> +local function format_bytes(bytes, config)
> +  if config.human_readable then
> +    return human_readable_bytes(bytes)
> +  else
> +    return string.format('%dB', bytes)
> +  end
> +end
> +
> +function M.render(events, config)

Side note: Maybe it's better to give just flag (is_human_readable) here
instead of the whole config, but I'm not sure... Feel free to ignore.

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 4/4] profilers: print user-friendly errors

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
Please consider my comments below.

On 04.12.23, Maxim Kokryashkin wrote:
> Prior to this patch, there was no error-checking in profilers,
> which resulted in raw Lua errors being reported in cases of
> non-existing paths or corrupt file structures. This patch adds
> error handling, so all parsing errors are now reported in a
> user-friendly manner.
> 
> Event parsing is now moved into a separate profiler-agnostic
> module.
> 
> Tool CLI flag tests are adapted correspondingly to error message
> changes.
> 
> Resolves tarantool/tarantool#9217
> Part of tarantool/tarantool#5994
> ---
>  .../gh-5688-tool-cli-flag.test.lua            |  4 +-
>  ...17-profile-parsers-error-handling.test.lua | 79 +++++++++++++++++++
>  tools/memprof.lua                             | 47 +++++++++--
>  tools/sysprof.lua                             | 42 ++++++++--
>  tools/sysprof/parse.lua                       |  2 +-
>  tools/utils/symtab.lua                        |  2 +-
>  6 files changed, 158 insertions(+), 18 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
> 
> diff --git a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
> index 75293f11..ec958031 100644
> --- a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
> +++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua

<snipped>

> diff --git a/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
> new file mode 100644
> index 00000000..9a818086
> --- /dev/null
> +++ b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
> @@ -0,0 +1,79 @@
> +local tap = require('tap')
> +local test = tap.test('gh-9217-profile-parsers-error-handling'):skipcond({
> +  ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and
> +                                                      jit.arch ~= 'x64',
> +  ['Profile tools are implemented for Linux only'] = jit.os ~= 'Linux',
> +  -- XXX: Tarantool integration is required to run this test properly.
> +  -- luacheck: no global
> +  ['No profile tools CLI option integration'] = _TARANTOOL,
> +})
> +
> +test:plan(6)

We can use here `#TEST_CASES * 2` instead of `6`.

> +

<snipped>

> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index acadbc17..a04608b8 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua
> @@ -10,11 +10,11 @@
>  -- Major portions taken verbatim or adapted from the LuaVela.
>  -- Copyright (C) 2015-2019 IPONWEB Ltd.
>  
> -local bufread = require "utils.bufread"
> -local memprof = require "memprof.parse"
> -local process = require "memprof.process"
> -local symtab = require "utils.symtab"
> -local view = require "memprof.humanize"
> +local bufread = require('utils.bufread')
> +local symtab = require('utils.symtab')
> +local memprof = require('memprof.parse')
> +local process = require('memprof.process')
> +local view = require('memprof.humanize')

This part of refactoring is excess within this patch.

>  
>  local stdout, stderr = io.stdout, io.stderr
>  local match, gmatch = string.match, string.gmatch
> @@ -106,10 +106,41 @@ local function parseargs(args)
>    return args[args.argn]
>  end
>  
> +local function make_error_handler(inputfile, fmt)

Looks like (fmt, inputfile) order is better.

> +  return function()

I'm a little bit confused that there is no verbose error description.
I suppose, that we should report the given error too.

> +    io.stderr:write(string.format(fmt, inputfile))
> +    os.exit(1, true)
> +  end
> +end
> +
> +local function safe_event_reader(inputfile)
> +  local _, reader = xpcall(
> +    bufread.new,
> +    make_error_handler(inputfile, 'Failed to open %s.'),
> +    inputfile
> +  )
> +
> +  local _, symbols = xpcall(
> +    symtab.parse,
> +    make_error_handler(inputfile, 'Failed to parse symtab from %s.'),
> +    reader
> +  )
> +
> +  local _, events = xpcall(
> +    memprof.parse,
> +    make_error_handler(inputfile, 'Failed to parse profile data from %s.'),
> +    reader,
> +    symbols
> +  )
> +  return events, symbols
> +end
> +
>  local function dump(inputfile)
> -  local reader = bufread.new(inputfile)
> -  local symbols = symtab.parse(reader)
> -  local events = memprof.parse(reader, symbols)
> +  -- XXX: This function exits with a non-zero exit code and
> +  -- prints an error message if it encounters any failure during
> +  -- the process of parsing.
> +  local events, symbols = safe_event_reader(inputfile)
> +
>    if not config.leak_only then
>      view.profile_info(events, config)
>    end
> diff --git a/tools/sysprof.lua b/tools/sysprof.lua
> index 8449b23f..d2efcd7f 100644
> --- a/tools/sysprof.lua
> +++ b/tools/sysprof.lua
> @@ -1,6 +1,6 @@
> -local bufread = require "utils.bufread"
> -local sysprof = require "sysprof.parse"
> -local symtab = require "utils.symtab"
> +local bufread = require('utils.bufread')
> +local symtab = require('utils.symtab')
> +local sysprof = require('sysprof.parse')

This part of refactoring is excess within this patch.

>  
>  local stdout, stderr = io.stdout, io.stderr
>  local match, gmatch = string.match, string.gmatch
> @@ -77,10 +77,40 @@ local function parseargs(args)
>    return args[args.argn]
>  end
>  
> +local function make_error_handler(inputfile, fmt)

Looks like (fmt, inputfile) order is better.

> +  return function()

I'm a little bit confused that there is no verbose error description.
I suppose, that we should report the given error too.

> +    io.stderr:write(string.format(fmt, inputfile))
> +    os.exit(1, true)
> +  end
> +end
> +
> +local function safe_event_reader(inputfile)
> +  local _, reader = xpcall(
> +    bufread.new,
> +    make_error_handler(inputfile, 'Failed to open %s.'),
> +    inputfile
> +  )
> +
> +  local _, symbols = xpcall(
> +    symtab.parse,
> +    make_error_handler(inputfile, 'Failed to parse symtab from %s.'),
> +    reader
> +  )
> +
> +  local _, events = xpcall(
> +    sysprof.parse,
> +    make_error_handler(inputfile, 'Failed to parse profile data from %s.'),
> +    reader,
> +    symbols
> +  )
> +  return events, symbols
> +end

This part is literally the same as for the sysprof parser (except the parser
itself). May be we should separate it from the parsers code?

> +
>  local function dump(inputfile)
> -  local reader = bufread.new(inputfile)
> -  local symbols = symtab.parse(reader)
> -  local events = sysprof.parse(reader, symbols)
> +  -- XXX: This function exits with a non-zero exit code and
> +  -- prints an error message if it encounters any failure during
> +  -- the process of parsing.
> +  local events = safe_event_reader(inputfile)
>  
>    for stack, count in pairs(events) do
>      print(stack, count)
> diff --git a/tools/sysprof/parse.lua b/tools/sysprof/parse.lua
> index 64c4a455..749f70db 100755
> --- a/tools/sysprof/parse.lua
> +++ b/tools/sysprof/parse.lua

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun