Hi, Sergey,

thanks for review!

Changes applied and force-pushed.

Sergey

Hi, Sergey,
Thanks for the fixes!
See my answers below.

On 25.02.25, Sergey Bronnikov wrote:

Hi, Sergey,

thanks for review! Updates applied and force-pushed.

Sergey

On 13.02.2025 18:31, Sergey Kaplun via Tarantool-patches wrote:

Hi, Sergey!
Thanks for the fixes and clarification!
Please consider by answers below.

On 12.02.25, Sergey Bronnikov wrote:

<snipped>

+local script = utils.exec.makecmd(arg)
+-- Execute a Lua script with start and stop LuaJIT profiler,
+-- it is expected no samples found by profiler. The script's
+-- output is suppressed, it is not interested.
+local _ = script(p_filename, p_postfix)
# > I don't get it. Why do we need the separate script for it?
# prof_finish() is executed first time on calling `jit.p.stop()`
#
# and executed second time when GC finalizer is calling [1] and therefore
# message "'[No samples collected]'" appears
#
# two times in the output file.
#
# 1.https://github.com/tarantool/luajit/blob/80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527/src/jit/p.lua#L290
#
# We can inline script.lua to a test body and call garbage collector manually,
# but I propose to leave it as is because  executing as a separate process is
# more real use case.

Now it is clear to me, thanks!

This is another face of the bug that was fixed by this commit.
Nice catch!

In the original issue, automated parsing shows empty file content.
I suppose it was done with something like the following:

| LUA_PATH="src/?.lua;;" src/luajit -e '
| local jp = require"jit.p"
| local filename = "/tmp/j.p"
|
| collectgarbage("stop")
| jp.start("i9999999", filename)
| jp.stop()
|
| local f = io.open(filename, "r")
| print(("<%s>"):format(f:read("*all")))
| f.close()
| collectgarbage("restart")
| os.remove(filename)
| '

Before the patch, I get the following output:

| <>

After the patch, I get the following output:

| <[No samples collected]
| >

ok, replaced testcase

+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1304

Nit: Missed dot at the end of the sentence.

Fixed, thanks!

(BTW currently 150 tests with links finished with dot, and 35 are not.)

<snipped>

+
+ jit_p.start('i9999999', filename)

Please add the comment before the call that we set the interval of the
profiling to a huge enough value to be sure that there are no samples
collected.

+ jit_p.stop()
+
+ local f = io.open(filename, 'r')
+ local p_content = io.open(filename):read('a*')
+ test:is(p_content, '[No samples collected]\n', 'profile dump has no samples')
+ f.close()

Typo: s/f./f:/

+
+ -- Teardown.
+ collectgarbage('restart')
+ os.remove(filename)

I guess this is the use case for guys from the NeoVim:
They run the profiler inside the application but have no samples
collected. The inspector may read the file from the different process,
but there is no need for it. And, yes, in the test we have to manually
stop and restart the GC, but only for the stability of the reproducer.
It is not a part of a use case itself.

Your test case may be simplified as the following (in terms of my
script):
| LUA_PATH="src/?.lua;;" src/luajit -e '
| local jp = require"jit.p"
| local filename = "/tmp/j.p"
|
| collectgarbage("stop")
| jp.start("i9999999", filename)
| jp.stop()
|
| -- Unload the module and clean the local.
| package.loaded["jit.p"] = nil
| jp = nil
| collectgarbage("collect")
|
| local f = io.open(filename, "r")
| print(("<%s>"):format(f:read("*all")))
| f.close()
| collectgarbage("restart")
| os.remove(filename)
| '
| <[No samples collected]
| [No samples collected]
| >

Since, IMHO, there is no need to check the </proc/$$/fd> content (see
the rationale below), we may use this test case in the original file
without the helper script.

So, let's check both cases (the original one and that found by you) in
the test.

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

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

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.

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>