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

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

Hi, Sergey!
Thanks for the patch!
LGTM, with a minor nits below.

On 13.02.25, Sergey Bronnikov wrote:
> sysprof has an optional parameter `path`, that set a path to

Typo: s/set/sets/

> profiling output file, by default the path is `sysprof.bin`.

Typo: s/profiling/the profiling/
Typo: s/file, by default/file. By 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 memprof profiling output file - `memprof.bin`.

Typo: s/memprof/the memprof/

> ---
>  src/lib_misc.c                                |  5 ++-
>  ...misclib-memprof-lapi-default-file.test.lua | 41 +++++++++++++++++++
>  2 files changed, 45 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 7666d85f..4f5d72fc 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..b6233d4a
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
> @@ -0,0 +1,41 @@
> +local tap = require("tap")

Minor: Let's use single quotes here and below.
Side note: This code style part wasn't fixed when the first tests for
memprof was written.

> +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"

Minor: Please use brackets for function calls.

> +
> +test:test("default-output-file", function(subtest)
> +
> +  subtest:plan(1)
> +
> +  local def_output_file = 'memprof.bin'

Minor: s/def/default/

> +  os.remove(def_output_file)
> +
> +  local res, err = misc.memprof.start()
> +  -- Should start successfully.
> +  assert(res, err)

This assert should be removed since we want to remove the file in case
of an error (for example, we can't write the memprof prologue).

> +
> +  res, err = misc.memprof.stop()
> +  -- Should stop successfully.
> +  assert(res, err)

This assert should be removed since we want to remove the file in case
of an error.

> +
> +  -- Want to cleanup carefully if something went wrong.
> +  if not res then
> +    os.remove(def_output_file)
> +    error(err)
> +  end
> +
> +  local profile_buf = tools.read_file(def_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(def_output_file)
> +end)
> +
> +test:done(true)
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun