Re: [Tarantool-patches] [PATCH] memprof: introduce a CLI flag to run dump parser

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

Hi, Maxim!

Thanks for the patch!

Please, consider my comments below.

On 29.07.21, Maxim Kokryashkin wrote:
> It is inconvenient to use a standalone shell script to parse a memprof
> dump. That is why this commit adds a CLI flag to call the parser into
> the LuaJIT, so now it is possible to parse a dump as simple as:
> ```
> luajit -m memprof.bin
> ```

And what about profile enabling/disabling?
How it scales for system profiler for example?

Yes, we should add new flag -m (stands for misc. builtin), but it should
work more similar like -j flag. So we can run memprof enabled (like
`-jp`). Also, we can use an **another** flag for tools (`-t`?) i.e. for
memprof data parsing.

The tricky part here is to come up with simple and user-friendly naming
for each (if we really need this) tool utility.

Thoughts?

> 
> Closes tarantool/tarantool#5688
> ---
>  CMakeLists.txt         |  8 +++++---
>  src/CMakeLists.txt     |  5 +++++
>  src/lj_tools_conf.h.in |  6 ++++++
>  src/luajit.c           | 36 ++++++++++++++++++++++++++++--------
>  tools/CMakeLists.txt   |  2 ++
>  5 files changed, 46 insertions(+), 11 deletions(-)
>  create mode 100644 src/lj_tools_conf.h.in
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 5348e043..619e9441 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -250,6 +250,11 @@ endif()
>  # related compiler and linker flags passed. This should be done
>  # the right way later.
>  
> +# --- Tools --------------------------------------------------------------------
> +
> +add_subdirectory(tools)
> +set(LUAJIT_TOOLS_DIR "${LUAJIT_TOOLS_DIR}")
> +
>  # --- Main source tree ---------------------------------------------------------
>  
>  add_subdirectory(src)
> @@ -258,9 +263,6 @@ add_subdirectory(src)
>  
>  add_subdirectory(etc)
>  
> -# --- Tools --------------------------------------------------------------------
> -
> -add_subdirectory(tools)

Why do we need this code movement?

>  
>  # --- Testing source tree ------------------------------------------------------
>  
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 809aac68..d9debccf 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -142,6 +142,8 @@ make_source_list(SOURCES_CORE_NO_JIT_FFI
>      ${SOURCES_UTILS}
>  )
>  
> +configure_file(${CMAKE_CURRENT_SOURCE_DIR}/lj_tools_conf.h.in ${CMAKE_CURRENT_SOURCE_DIR}/lj_tools_conf.h)
> +
>  set(SOURCES_CORE ${SOURCES_CORE_NO_JIT_FFI})
>  
>  # Build JIT sources if JIT support is enabled.
> @@ -248,6 +250,9 @@ add_custom_target(
>            jit/vmdef.lua
>  )
>  
> +# --- Generate luajit tools config header -------------------------------------
> +configure_file(${CMAKE_CURRENT_SOURCE_DIR}/lj_tools_conf.h.in ${CMAKE_CURRENT_SOURCE_DIR}/lj_tools_conf.h)
> +
>  # --- Generate core and VM object files ---------------------------------------
>  
>  # Virtual machine.
> diff --git a/src/lj_tools_conf.h.in b/src/lj_tools_conf.h.in
> new file mode 100644
> index 00000000..366c3ec4
> --- /dev/null
> +++ b/src/lj_tools_conf.h.in
> @@ -0,0 +1,6 @@
> +#ifndef LJ_TOOLS_CONF_H
> +#define LJ_TOOLS_CONF_H
> +
> +#define PARSER_PATH "@LUAJIT_TOOLS_DIR@/memprof.lua"
> +
> +#endif

For what do we need this file?

> diff --git a/src/luajit.c b/src/luajit.c
> index 1ca24301..6697d00f 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c
> @@ -19,6 +19,8 @@
>  
>  #include "lj_arch.h"
>  
> +#include "lj_tools_conf.h"
> +
>  #if LJ_TARGET_POSIX
>  #include <unistd.h>
>  #define lua_stdin_is_tty()	isatty(0)
> @@ -72,6 +74,7 @@ static void print_usage(void)
>    "  -O[opt]   Control LuaJIT optimizations.\n"
>    "  -i        Enter interactive mode after executing " LUA_QL("script") ".\n"
>    "  -v        Show version information.\n"
> +  "  -m        Parse memprof profile data.\n"
>    "  -E        Ignore environment variables.\n"
>    "  --        Stop handling options.\n"
>    "  -         Execute stdin and stop handling options.\n", stderr);
> @@ -266,21 +269,17 @@ static void dotty(lua_State *L)
>    progname = oldprogname;
>  }
>  
> -static int handle_script(lua_State *L, char **argx)
> +static int call_script(lua_State *L, const char *fname)
>  {
> -  int status;
> -  const char *fname = argx[0];
> -  if (strcmp(fname, "-") == 0 && strcmp(argx[-1], "--") != 0)
> -    fname = NULL;  /* stdin */
> -  status = luaL_loadfile(L, fname);
> +  int status = luaL_loadfile(L, fname);
>    if (status == LUA_OK) {
>      /* Fetch args from arg table. LUA_INIT or -e might have changed them. */
>      int narg = 0;
>      lua_getglobal(L, "arg");
>      if (lua_istable(L, -1)) {
>        do {
> -	narg++;
> -	lua_rawgeti(L, -narg, narg);
> +	      narg++;
> +	      lua_rawgeti(L, -narg, narg);
>        } while (!lua_isnil(L, -1));
>        lua_pop(L, 1);
>        lua_remove(L, -narg);
> @@ -290,6 +289,16 @@ static int handle_script(lua_State *L, char **argx)
>      }
>      status = docall(L, narg, 0);
>    }
> +  return status;
> +}
> +
> +static int handle_script(lua_State *L, char **argx)
> +{
> +  int status;
> +  const char *fname = argx[0];
> +  if (strcmp(fname, "-") == 0 && strcmp(argx[-1], "--") != 0)
> +    fname = NULL;  /* stdin */
> +  call_script(L, fname);
>    return report(L, status);
>  }
>  
> @@ -398,6 +407,7 @@ static int dobytecode(lua_State *L, char **argv)
>  #define FLAGS_EXEC		4
>  #define FLAGS_OPTION		8
>  #define FLAGS_NOENV		16
> +#define FLAGS_MEMPROF_PARSE 32

I believe, that we don't need a new flag only for memprof parsing.
It can be handled by FLAGS_OPTION as -j flag.
We can introduce the new function `domisccmd()` like the `dojitcmd()`.

>  
>  static int collectargs(char **argv, int *flags)
>  {
> @@ -411,6 +421,10 @@ static int collectargs(char **argv, int *flags)
>        return i+1;
>      case '\0':
>        return i;
> +    case 'm':
> +      notail(argv[i]);
> +      *flags |= FLAGS_MEMPROF_PARSE;
> +      return i;
>      case 'i':
>        notail(argv[i]);
>        *flags |= FLAGS_INTERACTIVE;
> @@ -547,6 +561,12 @@ static int pmain(lua_State *L)
>    s->status = runargs(L, argv, argn);
>    if (s->status != LUA_OK) return 0;
>  
> +  if((flags & FLAGS_MEMPROF_PARSE)) {
> +    //char* parser_path = "/usr/local/share/luajit-2.1.0-beta3/memprof.lua";
> +    s->status = report(L, call_script(L, PARSER_PATH));
> +    return s->status == LUA_OK;
> +  }
> +
>    if (s->argc > argn) {
>      s->status = handle_script(L, argv + argn);
>      if (s->status != LUA_OK) return 0;
> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index 61830e44..84129153 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt
> @@ -16,6 +16,7 @@ else()
>    # path where LuaJIT binary is located.
>    set(LUAJIT_TOOLS_BIN ${LUAJIT_BINARY_DIR}/${LUAJIT_CLI_NAME})
>    set(LUAJIT_TOOLS_DIR ${CMAKE_CURRENT_SOURCE_DIR})
> +  set(LUAJIT_TOOLS_DIR ${LUAJIT_TOOLS_DIR} PARENT_SCOPE)
>    # XXX: Unfortunately, there is no convenient way to set
>    # particular permissions to the output file via CMake.
>    # Furthermore, I even failed to copy the given file to the same
> @@ -75,6 +76,7 @@ else()
>      "
>        set(LUAJIT_TOOLS_BIN ${CMAKE_INSTALL_PREFIX}/bin/${LUAJIT_CLI_NAME})
>        set(LUAJIT_TOOLS_DIR ${CMAKE_INSTALL_PREFIX}/${LUAJIT_DATAROOTDIR})
> +      set(LUAJIT_TOOLS_DIR ${LUAJIT_TOOLS_DIR} PARENT_SCOPE)
>        configure_file(${CMAKE_CURRENT_SOURCE_DIR}/luajit-parse-memprof.in
>          ${PROJECT_BINARY_DIR}/luajit-parse-memprof @ONLY ESCAPE_QUOTES)
>        file(INSTALL ${PROJECT_BINARY_DIR}/luajit-parse-memprof
> -- 
> 2.32.0
> 

-- 
Best regards,
Sergey Kaplun