<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey,</p>
<p>thanks for review!</p>
<p>Changes applied and force-pushed.<br>
</p>
<p>Sergey<br>
</p>
<div class="moz-cite-prefix">On 26.02.2025 14:58, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z78B0BD8CeWNw0h2@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey,
Thanks for the fixes!
See my answers below.
On 25.02.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Hi, Sergey,
thanks for review! Updates applied and force-pushed.
Sergey
On 13.02.2025 18:31, Sergey Kaplun via Tarantool-patches wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the fixes and clarification!
Please consider by answers below.
On 12.02.25, Sergey Bronnikov wrote:
</pre>
</blockquote>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">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(<a class="moz-txt-link-freetext" href="f:read(">f:read(</a>"*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]
| >
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">ok, replaced testcase
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- See also: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1304">https://github.com/LuaJIT/LuaJIT/issues/1304</a>
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Nit: Missed dot at the end of the sentence.
</pre>
</blockquote>
<p>Fixed, thanks!</p>
<p>(BTW currently 150 tests with links finished with dot, and 35 are
not.)<br>
</p>
<blockquote type="cite" cite="mid:Z78B0BD8CeWNw0h2@root">
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+ jit_p.start('i9999999', filename)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.</pre>
</blockquote>
Added.<br>
<blockquote type="cite" cite="mid:Z78B0BD8CeWNw0h2@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ jit_p.stop()
+
+ local f = io.open(filename, 'r')
+ local p_content = io.open(filename):read('a*')
+ <a class="moz-txt-link-freetext" href="test:is(p_content">test:is(p_content</a>, '[No samples collected]\n', 'profile dump has no samples')
+ f.close()
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/f./<a class="moz-txt-link-freetext" href="f:/">f:/</a>
</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z78B0BD8CeWNw0h2@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+ -- Teardown.
+ collectgarbage('restart')
+ os.remove(filename)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
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(<a class="moz-txt-link-freetext" href="f:read(">f:read(</a>"*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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
What is a rationale for this?
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.
</pre>
</blockquote>
<p>How often users unload profiler's module? For me, this case looks
artificial</p>
<p>and the final goal for us is not a demonstration, but covering a
code touched by the patch,</p>
<p>it is done by proposed test.</p>
<p>(I'm really confused that on review I learn more and more new
requirements to the patches</p>
<p>like adding all available testcases that cover a patch or that
code should follow C89 or</p>
<p>that using etc.)</p>
<p>The bug fixed by the patch is quite minor, it will not break
production and will not kill humans etc.</p>
<p>I believe a single testcase and time that we both spend on doing
backport, test and two iterations</p>
<p>of review is more than enough for this patch. Moreover, jit.p is
not used by Tarantool users,</p>
<p>we provide our own profilers to them.<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Z78B0BD8CeWNw0h2@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>