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

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

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