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

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

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