[Tarantool-patches] [PATCH luajit 4/4] profilers: print user-friendly errors
Sergey Kaplun
skaplun at tarantool.org
Tue Dec 5 12:35:42 MSK 2023
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
More information about the Tarantool-patches
mailing list