Hi, Sergey,
thanks for review!
Fixed.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 outputTypo: s/to set/setting/ Typo: s/profiling/the profiling/
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