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

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

[Sergey Bronnikov via Tarantool-patches]

It is not allowed to call a function `sysprof.report()` without
stopping profiler. However, sometimes it may be useful to analyze
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
@@ -532,8 +532,6 @@ int lj_sysprof_stop(lua_State *L)
 int lj_sysprof_report(struct luam_Sysprof_Counters *counters)
 {
   const struct sysprof *sp = &sysprof;
-  if (sp->state != SPS_IDLE)
-    return PROFILE_ERRUSE;
   memcpy(counters, &sp->counters, sizeof(sp->counters));
   return PROFILE_SUCCESS;
 }
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
@@ -10,7 +10,7 @@ local test = tap.test("misclib-sysprof-lapi"):skipcond({
   ["Disabled due to #10803"] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
 })
 
-test:plan(44)
+test:plan(48)
 
 jit.off()
 -- XXX: Run JIT tuning functions in a safe frame to avoid errors
@@ -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",
+    interval = 1,
+    path = "/dev/null",
+}
+test:is(res, true, "res is correct")
+test:is(err, nil, "no error")
+test:is(errno, nil, "no errno")
+
+local report = misc.sysprof.report()
+test:ok(report.samples == 0, "total number of samples is non-zero")
+misc.sysprof.stop()
+
 -- DEFAULT MODE
 
 if not pcall(generate_output, { mode = "D", interval = 100 }) then
   test:fail('`default` mode with interval 100')
 end
 
-local report = misc.sysprof.report()
+report = misc.sysprof.report()
 
 -- Check the profile is not empty.
 test:ok(report.samples > 0,
-- 
2.43.0

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

[Sergey Kaplun via Tarantool-patches]

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/

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

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

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

> +misc.sysprof.stop()
> +

<snipped>

> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

Minor: The commit title is more than 50 symbols.
I suggest rephrasing it like the following:
| sysprof: allow calling sysprof.report before stop

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

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

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

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

[Sergey Bronnikov via Tarantool-patches]

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

Fixed, thanks!

On 6/4/25 11:36, Sergey Kaplun via Tarantool-patches wrote:
> Minor: The commit title is more than 50 symbols.
> I suggest rephrasing it like the following:
> | sysprof: allow calling sysprof.report before stop
>

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

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

[Sergey Kaplun via Tarantool-patches]

Sergey,
Thanks for the fixes!
LGTM, with the ignorable last nit below.

On 04.06.25, Sergey Bronnikov wrote:
> 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(-)
> >>

<snipped>

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

I would rather use
| assert(misc.sysprof.start({...})
and
| assert(misc.sysprof.stop())
instead, for simplicity and to avoid the irrelevant local variables.

Feel free to ignore.

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

Side note: Checking the non-0 samples for default payload instead.

> >> +misc.sysprof.stop()
> >> +
> > <snipped>
> >
> >> 2.43.0
> >>

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Hello, Sergey,

On 6/4/25 16:13, Sergey Kaplun wrote:


<snipped>

>>>> +    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 andtest:is() replaced with assert()
> I would rather use
> | assert(misc.sysprof.start({...})
> and
> | assert(misc.sysprof.stop())
> instead, for simplicity and to avoid the irrelevant local variables.
>
> Feel free to ignore.
Fixed.
>>>> +
>>>> +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.
> Side note: Checking the non-0 samples for default payload instead.
>
>>>> +misc.sysprof.stop()
>>>> +
>>> <snipped>
>>>
>>>> 2.43.0
>>>>

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