<!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!<br>
</p>
<div class="moz-cite-prefix">On 10.02.2025 17:51, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the patch!
LGTM, with a bunch of nits below.
On 04.02.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">The patch add descriptions to sysprof testcases to make TAP report
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/add/adds/
Typo: s/TAP/the TAP/
</pre>
</blockquote>
<p>Fixed both typos.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">more usable.
---
.../profilers/misclib-sysprof-lapi.test.lua | 36 ++++++++++---------
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index b2c38c49..581fb7fd 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -65,34 +65,35 @@ end
-- Wrong profiling mode.
local res, err, errno = misc.sysprof.start{ mode = "A" }
-test:ok(res == nil and err:match("profiler misuse"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("profiler misuse"), "res with no parameters")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: s/res/result status/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">
Here and below.
Feel free to ignore.
It is not the "no parameters", but rather the wrong profiler mode.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+test:ok(type(errno) == "number", "errno with no parameters")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
It is not the "no parameters", but rather the wrong profiler mode.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
-- Already running.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> -- Not running.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> -- Bad path.
res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
-test:ok(res == nil and err:match("No such file or directory"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("No such file or directory"), "res and error with bad path")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Line length is more than 80 symbols.</pre>
</blockquote>
Fixed and sent a separate patch that limits max len by 80 symbols.<br>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+test:ok(type(errno) == "number", "errno with bad path")
-- Bad interval.
res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
-test:ok(res == nil and err:match("profiler misuse"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("profiler misuse"), "res and error with bad interval")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Line length is more than 80 symbols.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+test:ok(type(errno) == "number", "errno with bad interval")
-- DEFAULT MODE
@@ -102,20 +103,23 @@ end
local report = misc.sysprof.report()
--- Check the profile is not empty
-test:ok(report.samples > 0)
+-- Check the profile is not empty.
+test:ok(report.samples > 0, "number of samples is greater than 0")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I would also add the following:
"... than 0 for the default payload"</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> -- There is a Lua function with FNEW bytecode in it. Hence there
-- are only three possible sample types:
-- * LFUNC -- Lua payload is sampled.
-- * GC -- Lua GC machinery triggered in scope of FNEW bytecode
-- is sampled.
-- * INTERP -- VM is in a specific state when the sample is taken.
-test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0)
+test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0,
+ "total number of LFUNC, GC and INTERP samples is greater than 0")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I would also add the following:
"... than 0 for the default payload"
</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> -- There is no fast functions and C function in default payload.
-test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0)
+test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0,
+ "total number of FFUNC and CFUNC samples is equal to 0")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I would also add the following:
"... to 0 for the default payload"</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> -- Check all JIT-related VM states are not sampled.
for _, vmstate in pairs({ 'TRACE', 'RECORD', 'OPT', 'ASM', 'EXIT' }) do
- test:ok(report.vmstate[vmstate] == 0)
+ local msg = ("total number of VM state %s is equal to 0"):format(vmstate)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I would also add the following:
"... to 0 for the default payload"
Minor: I would avoid the tmp variable here, since it is used only once
anyway.
</pre>
</blockquote>
Fixed and moved out of loop.<br>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ test:ok(report.vmstate[vmstate] == 0, msg)
end
-- With very big interval.
@@ -124,7 +128,7 @@ if not pcall(generate_output, { mode = "D", interval = 1000 }) then
end
report = misc.sysprof.report()
-test:ok(report.samples == 0)
+test:ok(report.samples == 0, "total number of samples is equal to 0")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I would also add the following:
"... to 0 for the too big sampling interval"
</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Z6oSWiBPS8h-xlzq@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
-- LEAF MODE
check_mode("L", 100)
--
2.34.1
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>