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!
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.

<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.

> 
> 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