Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

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

Hi, Sergey!
Thanks for the fixes, LGTM.

On 27.02.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for review!
> 
> Changes applied and force-pushed.
> 
> Sergey
> 
> On 26.02.2025 14:58, Sergey Kaplun wrote:

<snipped>

> >> What is a rationale for this?
> > As I said before, this is an additional possible demonstration of the
> > bug. I see nothing bad to add it as well, since it simulates a little
> > bit different workload -- flushing the output before we finish the
> > process or if we unload the module. I am not insisting on it, though.
> 
> How often users unload profiler's module? For me, this case looks artificial

The following module [1] is used to reload all Lua modules (including
`jit.p`). It was used for any Lua package update or code modification.

[1]: https://github.com/moonlibs/package-reload

Also, it is not only about unloading the module but also about finishing
the process too (as you first discovered). This looks like a real use
case. This variant of the test is just simpler than creating a child
process and checking its result files.

> and the final goal for us is not a demonstration, but covering a code 
> touched by the patch,
> 
> it is done by proposed test.
> 
> (I'm really confused that on review I learn more and more new 
> requirements to the patches

It is not a requirement, just a suggestion. See the last line of my
comment. I'm OK with reproducing the issue only since it is not the
vital part of the code.

> 
> like adding all available testcases that cover a patch or that code 
> should follow C89 or
> 
> that using etc.)
> 
> The bug fixed by the patch is quite minor, it will not break production 
> and will not kill humans etc.
> 
> I believe a single testcase and time that we both spend on doing 
> backport, test and two iterations
> 
> of review is more than enough for this patch. Moreover, jit.p is not 
> used by Tarantool users,
> 
> we provide our own profilers to them.

So, just ignore it then.

> 
> 
> >> This is a helper script for LuaJIT profiler and according to our
> >> backporting rules
> >>
> >> test must cover a backported patch. This rule is fulfilled by my test.
> > <snipped>
> >

-- 
Best regards,
Sergey Kaplun