<!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 18.02.2025 14:55, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z7R1FurubN_9WOiM@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the patch!
LGTM, with a minor nits below.
On 13.02.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">sysprof has an optional parameter `path`, that set a path to
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/set/sets/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z7R1FurubN_9WOiM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">profiling output file, by default the path is `sysprof.bin`.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/profiling/the profiling/
Typo: s/file, by default/file. By default,/
</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Z7R1FurubN_9WOiM@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:Z7R1FurubN_9WOiM@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 memprof profiling output file - `memprof.bin`.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/memprof/the memprof/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z7R1FurubN_9WOiM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">---
src/lib_misc.c | 5 ++-
...misclib-memprof-lapi-default-file.test.lua | 41 +++++++++++++++++++
2 files changed, 45 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 7666d85f..4f5d72fc 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..b6233d4a
--- /dev/null
+++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
@@ -0,0 +1,41 @@
+local tap = require("tap")
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Minor: Let's use single quotes here and below.
Side note: This code style part wasn't fixed when the first tests for
memprof was written.
</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z7R1FurubN_9WOiM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+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"
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Minor: Please use brackets for function calls.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z7R1FurubN_9WOiM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+test:test("default-output-file", function(subtest)
+
+ <a class="moz-txt-link-freetext" href="subtest:plan(1)">subtest:plan(1)</a>
+
+ local def_output_file = 'memprof.bin'
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Minor: s/def/default/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z7R1FurubN_9WOiM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ os.remove(def_output_file)
+
+ local res, err = misc.memprof.start()
+ -- Should start successfully.
+ assert(res, err)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
This assert should be removed since we want to remove the file in case
of an error (for example, we can't write the memprof prologue).</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Z7R1FurubN_9WOiM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+ res, err = misc.memprof.stop()
+ -- Should stop successfully.
+ assert(res, err)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
This assert should be removed since we want to remove the file in case
of an error.</pre>
</blockquote>
<p><br>
</p>
<p>Fixed.<br>
</p>
<blockquote type="cite" cite="mid:Z7R1FurubN_9WOiM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+ -- Want to cleanup carefully if something went wrong.
+ if not res then
+ os.remove(def_output_file)
+ error(err)
+ end
+
+ local profile_buf = tools.read_file(def_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(def_output_file)
+end)
+
+test:done(true)
--
2.34.1
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>