<!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 24.02.2025 14:14, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z7xUhi0sYr_0ZOuH@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the fixes!
LGTM, with a few small nits below.
On 20.02.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">sysprof has an optional parameter `path`, that sets a path to
the profiling output file. By default the path is `sysprof.bin`.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/default/default,/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z7xUhi0sYr_0ZOuH@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">`misc.memprof.start()` requires to set a path to profiling output
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/to set/setting/
Typo: s/profiling/the profiling/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z7xUhi0sYr_0ZOuH@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">file. The patch fixes this inconsistency by introducing a default
path to the memprof profiling output file - `memprof.bin`.
---
src/lib_misc.c | 5 ++-
...misclib-memprof-lapi-default-file.test.lua | 37 +++++++++++++++++++
2 files changed, 41 insertions(+), 1 deletion(-)
create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
diff --git a/src/lib_misc.c b/src/lib_misc.c
index d98cf3f0..92dc6e2a 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
new file mode 100644
index 00000000..ae8a73c9
--- /dev/null
+++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
@@ -0,0 +1,37 @@
+local tap = require('tap')
+local test = tap.test('misc-memprof-lapi-default-file'):skipcond({
+ ['Memprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
+ jit.arch ~= 'x64',
+ ['Memprof is disabled'] = os.getenv('LUAJIT_DISABLE_MEMPROF'),
+})
+
+test:plan(1)
+
+local tools = require('utils.tools')
+
+test:test('default-output-file', function(subtest)
+
+ <a class="moz-txt-link-freetext" href="subtest:plan(1)">subtest:plan(1)</a>
+
+ local default_output_file = 'memprof.bin'
+ os.remove(default_output_file)
+
+ local _, _ = misc.memprof.start()
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Minor: Do we want to check that the profiler starts successfully?
I suppose we should, since this is the expected behaviour for this
feature. In case the profiler is not started (old behaviour) we would
get an error from the branch below: profiler not running. This isn't a
verbose definition of what goes wrong.</pre>
</blockquote>
<p>I don't get why we should check that profiler is started in a
test for default output file.<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Z7xUhi0sYr_0ZOuH@root">
<pre wrap="" class="moz-quote-pre">
I suppose we may use `goto` here like the following:
| local res, err = misc.memprof.start()
| -- Should start successfully.
| if not res then
| goto err_handling
| end
|
| res, err = misc.memprof.stop()
|
| ::err_handling::
| -- Want to cleanup carefully if something went wrong.
| if not res then
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+ local res, err = misc.memprof.stop()
+
+ -- Want to cleanup carefully if something went wrong.
+ if not res then
+ os.remove(default_output_file)
+ error(err)
+ end
+
+ local profile_buf = tools.read_file(default_output_file)
+ <a class="moz-txt-link-freetext" href="subtest:ok(profile_buf">subtest:ok(profile_buf</a> ~= nil and #profile_buf ~= 0,
+ 'default output file is not empty')
+
+ -- We don't need it any more.
+ os.remove(default_output_file)
+end)
+
+test:done(true)
--
2.43.0
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>