Re: [Tarantool-patches] [PATCH luajit 1/2 v8] tools: add cli flag to run profile dump parsers

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the patch!
Please, consider my comments below.

On 25.08.23, Maxim Kokryashkin wrote:
> It is really inconvenient to use a standalone shell script to parse
> binary dumps from profilers. That is why this commit introduces a
> CLI flag for tools to the LuaJIT, so now it is possible to parse

Typo? s/for tools to the/for tools in the/

> a memprof dump as simple as:
> ```
> luajit -tm memprof.bin
> luajit -tm --leak-only memprof.bin
> ```
> 
> And the sysprof too:
> ```
> luajit -ts sysprof.bin
> luajit -ts --split sysprof.bin
> ```
> 
> The `luajit-parse-memprof` and `luajit-parse-sysprof` are purged
> as a result of this changes.

Typo: s/this/these/

> 
> Closes tarantool/tarantool#5688
> ---
> This patch requires additional changes for tarantool proxying,
> that are done in the PR.
>  Makefile.original                             |  20 ---
>  src/luajit.c                                  |  41 +++++-
>  .../gh-5688-tool-cli-flag.test.lua            | 130 ++++++++++++++++++
>  tools/CMakeLists.txt                          |  73 ----------
>  tools/luajit-parse-memprof.in                 |   6 -
>  tools/luajit-parse-sysprof.in                 |   6 -
>  tools/memprof.lua                             |   4 +-
>  tools/sysprof.lua                             |  16 ++-
>  8 files changed, 186 insertions(+), 110 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
>  delete mode 100755 tools/luajit-parse-memprof.in
>  delete mode 100644 tools/luajit-parse-sysprof.in
> 
> diff --git a/Makefile.original b/Makefile.original
> index 0c92df9e..10836f7d 100644
> --- a/Makefile.original
> +++ b/Makefile.original
> @@ -58,7 +58,6 @@ INSTALL_DYLIBSHORT2= libluajit-$(ABIVER).$(MAJVER).dylib
>  INSTALL_DYLIBNAME= libluajit-$(ABIVER).$(MAJVER).$(MINVER).$(RELVER).dylib
>  INSTALL_PCNAME= luajit.pc
>  INSTALL_TMEMPROFNAME= luajit-$(VERSION)-parse-memprof

I suppose, that this should be dropped too.

> -INSTALL_TMEMPROFSYMNAME= luajit-parse-memprof
>  
>  INSTALL_STATIC= $(INSTALL_LIB)/$(INSTALL_ANAME)
>  INSTALL_DYN= $(INSTALL_LIB)/$(INSTALL_SONAME)
> @@ -68,7 +67,6 @@ INSTALL_T= $(INSTALL_BIN)/$(INSTALL_TNAME)
>  INSTALL_TSYM= $(INSTALL_BIN)/$(INSTALL_TSYMNAME)
>  INSTALL_PC= $(INSTALL_PKGCONFIG)/$(INSTALL_PCNAME)
>  INSTALL_TMEMPROF= $(INSTALL_BIN)/$(INSTALL_TMEMPROFNAME)

As far as this one.

> -INSTALL_TMEMPROFSYM= $(INSTALL_BIN)/$(INSTALL_TMEMPROFSYMNAME)

<snipped>

> @@ -103,7 +99,6 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \
>  FILES_UTILSLIB= avl.lua bufread.lua symtab.lua
>  FILES_MEMPROFLIB= parse.lua humanize.lua

I noticed that this variable misses the <process.lua> recently
introduced. Also, there are no libs to install via sysprof too. I
suggest to fix this misbehaviour within the other patch (not even in
this patchset).

>  FILES_TOOLSLIB= memprof.lua
> -FILE_TMEMPROF= luajit-parse-memprof
>  

<snipped>

> diff --git a/src/luajit.c b/src/luajit.c
> index 1ca24301..e9ed89bc 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c

<snipped>

> +static int runtoolcmd(lua_State *L, const char *tool_name)
> +{
> +  lua_getglobal(L, "require");
> +  lua_pushstring(L, tool_name);
> +  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");

Nit: There is no need for new line here.

Also, I strongly believe that we should report an error in the case,
when there is another error reason. Just to avoid silent failures.

> +    return 1;
> +  }
> +  lua_getglobal(L, "arg");
> +  return report(L, lua_pcall(L, 1, 1, 0));
> +}
> +

<snipped>

> diff --git a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
> new file mode 100644
> index 00000000..b91902fd
> --- /dev/null
> +++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua

<snipped>

> +
> +local function tool_test_case(case_name, cmd_set)
> +  test:test(case_name, function(subtest)
> +    subtest:plan(#cmd_set)
> +
> +    for idx = 1, #cmd_set do
> +      local result = os.execute(cmd_set[idx].cmd .. SUPPRESS_OUTPUT) == 0

Here we got a problem: if sysprof has bad arguments, sysprof reports
nothing -- just silently fails:

| >>> LUA_PATH="tools/?.lua;;" src/luajit -ts /tmp/memprof.bin
| 13:29:14 jobs:0 burii@root:~/reviews/luajit/cli-flags$[1]
| >>> LUA_PATH="tools/?.lua;;" src/luajit -ts asdfl
| 13:29:14 jobs:0 burii@root:~/reviews/luajit/cli-flags$[1]

I suggest to check the error message too.

> +      subtest:ok(result == cmd_set[idx].expected, cmd_set[idx].cmd)
> +    end
> +  end)
> +end
> +
> +tool_test_case('smoke', SMOKE_CMD_SET)
> +
> +generate_profiler_output(TMP_BINFILE_MEMPROF, memprof_payload, misc.memprof)
> +tool_test_case('memprof parsing tool', MEMPROF_CMD_SET)
> +
> +local sysprof_opts = { mode = 'C', path = TMP_BINFILE_SYSPROF }
> +generate_profiler_output(sysprof_opts, sysprof_payload, misc.sysprof)
> +tool_test_case('sysprof parsing tool', SYSPROF_CMD_SET)
> +
> +os.remove(TMP_BINFILE_MEMPROF)
> +os.remove(TMP_BINFILE_SYSPROF)
> +test:done(true)
> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index dd7ec6bd..b7aeb472 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt

<snipped>

>    add_custom_target(tools-parse-memprof EXCLUDE_FROM_ALL DEPENDS
> -    luajit-parse-memprof
>      memprof/humanize.lua
>      memprof/parse.lua
>      memprof.lua

<memprof/process.lua> is missing here.

> @@ -66,27 +51,6 @@ else()
>        WORLD_READ
>      COMPONENT tools-parse-memprof
>    )

<snipped>

>    add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS
> -    luajit-parse-sysprof
>      sysprof/parse.lua
>      sysprof/collapse.lua
>      sysprof.lua
> @@ -148,27 +96,6 @@ else()
>        WORLD_READ
>      COMPONENT tools-parse-sysprof
>    )

<snipped>

> diff --git a/tools/luajit-parse-memprof.in b/tools/luajit-parse-memprof.in
> deleted file mode 100755
> index 8867202a..00000000
> --- a/tools/luajit-parse-memprof.in
> +++ /dev/null

<snipped>

> diff --git a/tools/luajit-parse-sysprof.in b/tools/luajit-parse-sysprof.in
> deleted file mode 100644
> index 2be25eb3..00000000
> --- a/tools/luajit-parse-sysprof.in
> +++ /dev/null

<snipped>

> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index cf66dd9e..8df1630a 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua

<snipped>

> +-- XXX: When this script is used as a preloaded module by an
> +-- application, it should return one function for correct parsing
> +-- of command line flags like --leak-only and dumping profile
> +-- info.
> +local function dump_wrapped(...)
> +  return dump(parseargs(...))
>  end
>  
>  -- FIXME: this script should be application-independent.
>  local args = {...}
>  if #args == 1 and args[1] == "sysprof" then
> -  return dump
> +  return dump_wrapped
>  else
> -  dump(parseargs(args))
> +  dump_wrapped(args)

Do we ever need this case since there is no direct call to it?
I suppose that this FIXME and if/else branches may be deleted.
Now we may always return the `dump_wrapped()` function.

>  end
> -- 
> 2.41.0
> 

-- 
Best regards,
Sergey Kaplun