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: 3103 bytes --]

Hi, Sergey!

Updated and force-pushed.

Sergey

On 05.03.2025 13:57, Sergey Kaplun wrote:
> Hi, Sergey!
> See my answer below.
>
> On 24.02.25, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for review!
> <snipped>
>
>>>> ---
>>>> +
>>>> +  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.
> If the `memprof.start()` call will fail in the test (for any reason),
> the next call of `memprof.stop()` will return `false, "profiler is not
> running"`. So, when debugging the test, we have no clue about the reason
> for the not-started profiler, which is not very convenient.

The patch below handles error from memprof start and stop:

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
index ae8a73c9..55b5c60b 100644
--- 
a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
+++ 
b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
@@ -16,21 +16,26 @@ test:test('default-output-file', function(subtest)
    local default_output_file = 'memprof.bin'
    os.remove(default_output_file)

-  local _, _ = misc.memprof.start()
-
-  local res, err = misc.memprof.stop()
+  local res, err = misc.memprof.start()
+  -- Want to cleanup carefully if something went wrong.
+  if not res then
+ test:fail('sysprof was not started: ' .. err)
+    os.remove(default_output_file)
+  end

+  res, err = misc.memprof.stop()
    -- Want to cleanup carefully if something went wrong.
    if not res then
+ test:fail('sysprof was not started: ' .. err)
      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.
+  -- We don't need it anymore.
    os.remove(default_output_file)
  end)


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

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