Hi, Sergey,

thanks for review!

On 24.02.2025 14:14, Sergey Kaplun via Tarantool-patches wrote:
Hi, Sergey!
Thanks for the fixes!
LGTM, with a few small nits below.

On 20.02.25, Sergey Bronnikov wrote:
sysprof has an optional parameter `path`, that sets a path to
the profiling output file. By default the path is `sysprof.bin`.
Typo: s/default/default,/
Fixed.

`misc.memprof.start()` requires to set a path to profiling output
Typo: s/to set/setting/
Typo: s/profiling/the profiling/
Fixed.

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
<snipped>

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)
+
+  subtest:plan(1)
+
+  local default_output_file = 'memprof.bin'
+  os.remove(default_output_file)
+
+  local _, _ = misc.memprof.start()
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.

I don't get why we should check that profiler is started in a test for default output file.


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

+
+  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)
+  subtest:ok(profile_buf ~= 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