Re: [Tarantool-patches] [PATCH luajit v2 5/6] memprof: introduce the `--human-readable` option

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

Thanks for the patch!

LGTM

On 12/6/23 21:55, 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   | 73 +++++++++++++++++++
>   tools/memprof.lua                             | 20 +++--
>   tools/memprof/humanize.lua                    | 46 +++++++++---
>   3 files changed, 123 insertions(+), 16 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..e34291be
> --- /dev/null
> +++ b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
> @@ -0,0 +1,73 @@
> +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,
> +})
> +
> +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.
> +}
> +
> +test:plan(#TEST_SET)
> +
> +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 955a1327..537cb869 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
> @@ -34,14 +40,18 @@ luajit-parse-memprof [options] memprof.bin
>   Supported options are:
>   
>     --help                            Show this help and exit
> +  --human-readable                  Use KiB/MiB/GiB notation instead of bytes
>     --leak-only                       Report only leaks information
>   ]]
>     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 +111,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)
> +  if not config.leak_only then
> +    view.profile_info(events, config)
>     end
>     local dheap = process.form_heap_delta(events)
> -  view.leak_info(dheap)
> +  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..a0b1693a 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 magnitude = 1
> +
> +  while bytes >= 1024 and magnitude < #units do
> +    bytes = bytes / 1024
> +    magnitude = magnitude + 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[magnitude])
> +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