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

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

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

Hi, Sergey,

thanks for review!

On 24.02.2025 14:14, Sergey Kaplun via Tarantool-patches wrote:
> 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,/
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 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 don't get why we should check that profiler is started in a test for 
default output file.


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

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