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

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

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

Hi, Sergey!

The second testcase has been added and

changes force-pushed to the branch.

Sergey

On 04.03.2025 18:40, Sergey Kaplun wrote:
> 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>
>>>

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