Hi, Sergey,
thanks for review!
Fixed.Hi, Sergey! Thanks for the patch! LGTM, with a minor nits below. On 13.02.25, Sergey Bronnikov wrote:sysprof has an optional parameter `path`, that set a path toTypo: s/set/sets/
profiling output file, by default the path is `sysprof.bin`.Typo: s/profiling/the profiling/ Typo: s/file, by default/file. By default,/
Fixed.
Fixed.`misc.memprof.start()` requires to set a path to profiling outputTypo: s/to set/setting/ Typo: s/profiling/the profiling/
Fixed.file. The patch fixes this inconsistency by introducing a default path to memprof profiling output file - `memprof.bin`.Typo: s/memprof/the memprof/
Fixed.--- 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<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..b6233d4a --- /dev/null +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua @@ -0,0 +1,41 @@ +local tap = require("tap")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.
Fixed.+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"Minor: Please use brackets for function calls.
Fixed.+ +test:test("default-output-file", function(subtest) + + subtest:plan(1) + + local def_output_file = 'memprof.bin'Minor: s/def/default/
Fixed.+ os.remove(def_output_file) + + local res, err = misc.memprof.start() + -- Should start successfully. + assert(res, err)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).
+ + res, err = misc.memprof.stop() + -- Should stop successfully. + assert(res, err)This assert should be removed since we want to remove the file in case of an error.
Fixed.
+ + -- 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) + subtest:ok(profile_buf ~= 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