[Tarantool-patches] [PATCH luajit v6] memprof: introduce cli flag to run dump parser
Sergey Kaplun
skaplun at tarantool.org
Sun Jul 9 15:12:38 MSK 2023
Hi, Maxim!
Thanks for the fixes!
LGTM, after fixing the comments below.
On 05.07.23, Maxim Kokryashkin wrote:
> It is really inconvenient to use a standalone shell script to parse
> memprof dump. That is why this commit introduces a CLI flag for tools
> to the LuaJIT, so now it is possible to parse memprof dump
> as simple as:
> ```
> luajit -tm memprof.bin
> ```
>
> Closes tarantool/tarantool#5688
> ---
> Changes in v6:
> - Simplified the patch.
It's really nice that we don't use any tricks with the environment now!
Good job!
>
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-5688-cli-for-memprof-parse
> PR: https://github.com/tarantool/tarantool/pull/8002
>
> src/luajit.c | 31 ++++++++++-
> .../gh-5688-memprof-cli-flag.test.lua | 55 +++++++++++++++++++
> 2 files changed, 85 insertions(+), 1 deletion(-)
> create mode 100644 test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
>
> diff --git a/src/luajit.c b/src/luajit.c
> index 1ca24301..81c0f8af 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c
<snipped>
> +static int dotoolcmd(lua_State *L, const char *cmd)
> +{
> + if(strcmp(cmd, "m") == 0) {
Typo: s<if(><if (>
If run with invalid cli flag value there is no error message shown.
| >>> src/luajit -tblabla /tmp/t.lua
| 14:56:56 jobs:0 burii at root:~/reviews/luajit/cli-flags$[1]
It will be nice to see some test for it.
> + lua_getglobal(L, "require");
> + lua_pushliteral(L, "memprof");
> + if (lua_pcall(L, 1, 1, 0)) {
> + const char *msg = lua_tostring(L, -1);
> + if (msg && !strncmp(msg, "module ", 7))
> + l_message(progname,
> + "unknown luaJIT command or tools not installed");
May be it is better to say that:
"tools.* modules not installed", since it isn't LuaJIT command.
> + return 1;
> + }
> + lua_getglobal(L, "arg");
> + return report(L, lua_pcall(L, 1, 1, 0));
> + }
> + return -1;
> +}
> +
<snipped>
> diff --git a/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua b/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
> new file mode 100644
> index 00000000..ba0cad68
> --- /dev/null
> +++ b/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
> @@ -0,0 +1,55 @@
> +local tap = require('tap')
> +local test = tap.test('gh-5688-cli-for-memprof-parse'):skipcond({
> + ['Memprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
> + jit.arch ~= 'x64',
> + ['Memprof is implemented for Linux only'] = jit.os ~= 'Linux',
Side note: I suppose that this is because memprof use symbol table dump
for C functions. But there is no table dumped for OSX. But this isn't a
reason to restrict memprof usage for this platform -- we can still
report C symbols as raw addresses, like it was done before. May you,
please, create a ticket for it (or for dumping C symbols for OSX as well
if you prefer this way :))?
> +-- XXX: The patch is for LuaJIT only, and it doesn't
<snipped>
> --
> 2.40.1
>
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list