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

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

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