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

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

[-- Attachment #1: Type: text/plain, Size: 3249 bytes --]

Hi, Sergey,

thanks for review!

On 18.02.2025 14:55, Sergey Kaplun via Tarantool-patches wrote:
> 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/
Fixed.
>
>> profiling output file, by default the path is `sysprof.bin`.
> Typo: s/profiling/the profiling/
> Typo: s/file, by default/file. By default,/

Fixed.


>> `misc.memprof.start()` requires to set a path to profiling output
> Typo: s/to set/setting/
> Typo: s/profiling/the profiling/
Fixed.
>
>> file. The patch fixes this inconsistency by introducing a default
>> path to memprof profiling output file - `memprof.bin`.
> Typo: s/memprof/the memprof/
Fixed.
>
>> ---
>>   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.
Fixed.
>> +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.
Fixed.
>
>> +
>> +test:test("default-output-file", function(subtest)
>> +
>> +subtest:plan(1)
>> +
>> +  local def_output_file = 'memprof.bin'
> Minor: s/def/default/
Fixed.
>
>> +  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).
Fixed.
>
>> +
>> +  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.


Fixed.

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

[-- Attachment #2: Type: text/html, Size: 6506 bytes --]