<!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>Hello, Sergey,</p>
<p>thanks for review! Please see my comments.</p>
<p>Updated version was force-pushed to the branch.</p>
<p>Sergey<br>
</p>
<div class="moz-cite-prefix">On 11.02.2025 17:53, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On 10.02.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">From: Mike Pall <mike>
Reported by Guilherme Batalheiro.
(cherry picked from commit fca66335d131669cf017420af6963a7565babb58)
Before the patch, a function `prof_finish` wrote a string
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: s/`prof_finish`/`prof_finish()`/
Feel free to ignore.
</pre>
</blockquote>
What for? I'm already written it is a function. Ignored.<br>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">`No samples collected` to profiler output file and then exits.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/profiler/a profiler/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
Typo: s/exits/exited/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Due to early exit, output file handle stay opened. This patch
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/output/the output/
Typo: s/opened/open/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">fixes condition and file handle is closed even a number of samples
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/condition/the condition/
Typo: s/file/the file/
Typo: s/even a number/even if the number/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">is equal to 0.
Sergey Bronnikov:
* added the description and the test for the problem
Part of tarantool/tarantool#11055
---
Branch: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-close-file-profiler">https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-close-file-profiler</a>
Related issues:
- <a class="moz-txt-link-freetext" href="https://github.com/luajIT/luajIT/issues/1304">https://github.com/luajIT/luajIT/issues/1304</a>
- <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/11055">https://github.com/tarantool/tarantool/issues/11055</a>
src/jit/p.lua | 4 +--
...close-profile-dump-with-0-samples.test.lua | 30 +++++++++++++++++++
.../script.lua | 14 +++++++++
test/tarantool-tests/utils/tools.lua | 4 +++
4 files changed, 49 insertions(+), 3 deletions(-)
create mode 100644 test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
create mode 100644 test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
diff --git a/src/jit/p.lua b/src/jit/p.lua
index 4569d69e..89b49584 100644
--- a/src/jit/p.lua
+++ b/src/jit/p.lua
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">diff --git a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
new file mode 100644
index 00000000..b50b5fce
--- /dev/null
+++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
@@ -0,0 +1,30 @@
+local tap = require('tap')
+local test = tap.test('lj-1304-close-profile-dump-with-0-samples'):skipcond({
+ ['Test requires /proc filesystem'] = jit.os == 'OSX',
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I suppose we may get off this skipcond, see the last comment.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+})
+local utils = require('utils')
+
+test:plan(1)
+
+-- Test file to demonstrate LuaJIT incorrect behaviour with missed
+-- close a file handle for profile output file.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo? s/close/closing/
Typo: s/profile/the profile/</pre>
</blockquote>
<p>Fixed:</p>
<p>@@ -7,10 +7,10 @@ local utils = require('utils')<br>
test:plan(1)<br>
<br>
-- Test file to demonstrate LuaJIT incorrect behaviour with
missed<br>
--- close a file handle for profile output file.<br>
+-- closing a file handle for the profile output file.<br>
-- See also: <a class="moz-txt-link-freetext" href="https://github.com/luajIT/luajIT/issues/1304">https://github.com/luajIT/luajIT/issues/1304</a><br>
<br>
-local p_filename = '/tmp/profile'<br>
+local p_filename = utils.tools.profilename<br>
<br>
-- <makecmd> runs %testname%/script.lua by
<LUAJIT_TEST_BINARY><br>
-- with the given environment, launch options and CLI arguments.<br>
</p>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+-- See also: <a class="moz-txt-link-freetext" href="https://github.com/luajIT/luajIT/issues/1304">https://github.com/luajIT/luajIT/issues/1304</a>
+
+local p_filename = '/tmp/profile'
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
It's better to use for the profile name generation the following
function:
| local profilename = require("utils").tools.profilename</pre>
</blockquote>
<p>Fixed:</p>
<p><br>
</p>
---
a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua<br>
+++
b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua<br>
@@ -10,7 +10,7 @@ test:plan(1)<br>
-- close a file handle for profile output file.<br>
-- See also: <a class="moz-txt-link-freetext" href="https://github.com/luajIT/luajIT/issues/1304">https://github.com/luajIT/luajIT/issues/1304</a><br>
<br>
-local p_filename = '/tmp/profile'<br>
+local p_filename = utils.tools.profilename<br>
<br>
-- <makecmd> runs %testname%/script.lua by
<LUAJIT_TEST_BINARY><br>
-- with the given environment, launch options and CLI arguments.<br>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+-- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
+-- with the given environment, launch options and CLI arguments.
+local script = utils.exec.makecmd(arg)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I don't get it. Why do we need the separate script for it?</pre>
</blockquote>
<p><br>
</p>
<p>prof_finish() is executed first time on calling `jit.p.stop()`</p>
<p>and executed second time when GC finalizer is calling [1] and
therefore message "'[No samples collected]'" appears</p>
<p>two times in the output file.</p>
<p>1.
<a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/blob/80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527/src/jit/p.lua#L290">https://github.com/tarantool/luajit/blob/80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527/src/jit/p.lua#L290</a></p>
<p><br>
</p>
<p>We can inline script.lua to a test body and call garbage
collector manually,</p>
<p>but I propose to leave it as is because executing as a separate
process is more real use case.<br>
</p>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+-- 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)
+
+local p_content = io.open(p_filename):read('a*')
+test:is(utils.tools.trim(p_content), '[No samples collected]',
+ 'profile dump has no samples')
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: I suppose simple `test:like()` will be enough. In that case there
is no need to use the newly introduced `trim()` function here.</pre>
</blockquote>
<p>The check checks that exactly one "[No samples collected]" is in
a file.</p>
<p>Before the patch, a file contains two "[No samples collected]".<br>
</p>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+-- Teardown.
+os.remove(p_filename)
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
new file mode 100644
index 00000000..77335a08
--- /dev/null
+++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
@@ -0,0 +1,14 @@
+local p_options = 'ri1'
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
It is better to use a much bigger interval (`i99999` for example) to be
sure that there will be no samples collected. Also, the `r` option isn't
required.
</pre>
</blockquote>
<p>Fixed:</p>
<p>---
a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua<br>
+++
b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua<br>
@@ -1,6 +1,8 @@<br>
local jit_p = require('jit.p')<br>
<br>
-local p_options = 'ri1'<br>
+-- Using a bigger interval to make sure that there will be no<br>
+-- samples collected.<br>
+local p_options = 'i99999'<br>
local p_filename = assert(arg[1], 'filename argument is missing')<br>
<br>
jit_p.start(p_options, p_filename)<br>
</p>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+local p_filename = assert(arg[1], 'filename argument is missing')
+
+require('jit.p').start(p_options, p_filename)
+
+-- No code to generate profiling samples.
+
+-- Stop profiler to execute `jit/p.lua:prof_fmt()`. With zero
+-- samples it triggers early return without closing the file.
+require('jit.p').stop()
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I suppose we may require this module only once.</pre>
</blockquote>
<p>Fixed:</p>
<p>---
a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua<br>
+++
b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua<br>
@@ -1,13 +1,15 @@<br>
+local jit_p = require('jit.p')<br>
+<br>
local p_options = 'ri1'<br>
local p_filename = assert(arg[1], 'filename argument is missing')<br>
<br>
-require('jit.p').start(p_options, p_filename)<br>
+jit_p.start(p_options, p_filename)<br>
<br>
-- No code to generate profiling samples.<br>
<br>
-- Stop profiler to execute `jit/p.lua:prof_fmt()`. With zero<br>
-- samples it triggers early return without closing the file.<br>
-require('jit.p').stop()<br>
+jit_p.stop()<br>
<br>
-- Make sure LuaJIT profiler is close a file handle.<br>
local ls_output = io.popen('ls -l /proc/$$/fd'):read('a*')<br>
</p>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+-- Make sure LuaJIT profiler is close a file handle.
+local ls_output = io.popen('ls -l /proc/$$/fd'):read('a*')
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: I am not sure that we really need this check. The descriptor will
be closed when it will be garbage collected, IINM, so there is no leak,
actually. I suggest dropping it and checking only the content of the
file -- this is the main issue regarding `jit.p` solved by that commit.</pre>
</blockquote>
<p>This is for checking that `prof_finish` behaves correctly.</p>
<p>fd leak is a resource leak and there is a separate CWE for this -</p>
<p><a class="moz-txt-link-freetext" href="https://cwe.mitre.org/data/definitions/775.html">https://cwe.mitre.org/data/definitions/775.html</a></p>
<p>On my machine max number of file descriptors is 126693 per
process:</p>
<p>[0] ~ $ ulimit -n<br>
126693</p>
<p>You will get file descriptor exhaustion if you will run</p>
<p>`require('jit.p').start()` 126693 times with different output
files.<br>
</p>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
Also, in this case we can run this test on OSX.</pre>
</blockquote>
Testing on MacOS doesn't add more value, because the problem is
platform-independent.<br>
<blockquote type="cite" cite="mid:Z6tkbodkFqJVv55r@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+assert(ls_output:find(p_filename) == nil, 'file is open')
diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua
index 33fcae78..9cb65daf 100644
--- a/test/tarantool-tests/utils/tools.lua
+++ b/test/tarantool-tests/utils/tools.lua
@@ -21,4 +21,8 @@ function M.read_file(path)
return content
end
+function M.trim(str)
+ return (str:gsub('^%s*(.-)%s*$', '%1'))
+end
+
return M
--
2.34.1
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>