[Tarantool-patches] [PATCH luajit 3/4] memprof: introduce the `--human-readable` option
Sergey Kaplun
skaplun at tarantool.org
Tue Dec 5 12:19:07 MSK 2023
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
More information about the Tarantool-patches
mailing list