Re: [Tarantool-patches] [PATCH luajit 8/8][v3] memprof: set default path to profiling output file

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

Hi, Sergey!
Thanks for the fixes!
LGTM, with a few small nits below.

On 20.02.25, Sergey Bronnikov wrote:
> sysprof has an optional parameter `path`, that sets a path to
> the profiling output file. By default the path is `sysprof.bin`.

Typo: s/default/default,/

> `misc.memprof.start()` requires to set a path to profiling output

Typo: s/to set/setting/
Typo: s/profiling/the profiling/

> file. The patch fixes this inconsistency by introducing a default
> path to the memprof profiling output file - `memprof.bin`.
> ---
>  src/lib_misc.c                                |  5 ++-
>  ...misclib-memprof-lapi-default-file.test.lua | 37 +++++++++++++++++++
>  2 files changed, 41 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
> 
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index d98cf3f0..92dc6e2a 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c

<snipped>

> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
> new file mode 100644
> index 00000000..ae8a73c9
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
> @@ -0,0 +1,37 @@
> +local tap = require('tap')
> +local test = tap.test('misc-memprof-lapi-default-file'):skipcond({
> +  ['Memprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
> +                                               jit.arch ~= 'x64',
> +  ['Memprof is disabled'] = os.getenv('LUAJIT_DISABLE_MEMPROF'),
> +})
> +
> +test:plan(1)
> +
> +local tools = require('utils.tools')
> +
> +test:test('default-output-file', function(subtest)
> +
> +  subtest:plan(1)
> +
> +  local default_output_file = 'memprof.bin'
> +  os.remove(default_output_file)
> +
> +  local _, _ = misc.memprof.start()

Minor: Do we want to check that the profiler starts successfully?
I suppose we should, since this is the expected behaviour for this
feature. In case the profiler is not started (old behaviour) we would
get an error from the branch below: profiler not running. This isn't a
verbose definition of what goes wrong.
I suppose we may use `goto` here like the following:

|   local res, err = misc.memprof.start()
|   -- Should start successfully.
|   if not res then
|     goto err_handling
|   end
|
|   res, err = misc.memprof.stop()
|
| ::err_handling::
|   -- Want to cleanup carefully if something went wrong.
|   if not res then

> +
> +  local res, err = misc.memprof.stop()
> +
> +  -- Want to cleanup carefully if something went wrong.
> +  if not res then
> +    os.remove(default_output_file)
> +    error(err)
> +  end
> +
> +  local profile_buf = tools.read_file(default_output_file)
> +  subtest:ok(profile_buf ~= nil and #profile_buf ~= 0,
> +             'default output file is not empty')
> +
> +  -- We don't need it any more.
> +  os.remove(default_output_file)
> +end)
> +
> +test:done(true)
> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun