[Tarantool-patches] [PATCH luajit][v1] sysprof: allow calling sysprof.report before stopping

[Sergey Bronnikov]

Hi, Sergey,

thanks for the comments! See my replies below.


On 6/4/25 11:35, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, with minor comments below.
>
> On 16.05.25, Sergey Bronnikov wrote:
>> It is not allowed to call a function `sysprof.report()` without
>> stopping profiler. However, sometimes it may be useful to analyze
> Typo: s/profiler/the profiler/
Fixed.
>> numbers provided by the report without stopping the profiler. The
>> patch removes the appropriate condition and allows reporting
>> without stopping.
>>
>> Resolves tarantool/tarantool#11229
>> ---
>> Branch:https://github.com/tarantool/luajit/tree/ligurio/gh-11229-misc.sysprof.report
>> Issue:https://github.com/tarantool/tarantool/issues/11229
>>
>>   src/lj_sysprof.c                               |  2 --
>>   .../profilers/misclib-sysprof-lapi.test.lua    | 18 ++++++++++++++++--
>>   2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
>> index 88c7a41b..cf6161a5 100644
>> --- a/src/lj_sysprof.c
>> +++ b/src/lj_sysprof.c
> <snipped>
>
>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> index f316c390..3e774a53 100644
>> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> <snipped>
>
>> @@ -166,13 +166,27 @@test:is(err, nil, "no error with good interval 1")
>>   test:is(errno, nil, "no errno with good interval 1")
>>   misc.sysprof.stop()
>>   
>> +-- Intermediate sysprof.report().
>> +res, err, errno = misc.sysprof.start{
>> +    mode = "C",
> I suppose we don't need to run the profiler in "C" mode. Let's use "D"
> here.

Fixed.

--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -168,7 +168,7 @@ misc.sysprof.stop()

  -- Intermediate sysprof.report().
  res, err, errno = misc.sysprof.start{
-    mode = "C",
+    mode = "D",
      interval = 1,
      path = "/dev/null",
  }

>
>> +    interval = 1,
>> +    path = "/dev/null",
>> +}
>> +test:is(res, true, "res is correct")
>> +test:is(err, nil, "no error")
>> +test:is(errno, nil, "no errno")
> I suppose that 2 last checks are excess. The first one is enough to be
> sure that the profiler is started. Also, we may use `assert()` here
> instead of `test:is()` check, since we don't want to _test_ the starting of
> the profiler only to _assert_ that the sysprof has been started.
>
last two checks were removed and test:is() replaced with assert()
>> +
>> +local report = misc.sysprof.report()
>> +test:ok(report.samples == 0, "total number of samples is non-zero")
> I'm not sure that this will always be true (for example, in coverage
> workflow). I suggest increasing the interval dramatically to avoid false
> positives here.
Updated.
>> +misc.sysprof.stop()
>> +
> <snipped>
>
>> 2.43.0
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20250604/91ca7a38/attachment.htm>