Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit v3] memprof: introduce cli flag to run dump parser
@ 2022-12-05 18:01 Maxim Kokryashkin via Tarantool-patches
  2023-02-07  7:23 ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 2+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-12-05 18:01 UTC (permalink / raw)
  To: tarantool-patches, sergos, skaplun

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 v3:
- Fixed comments as per review by Sergey

Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-5688-cli-for-memprof-parse
PR: https://github.com/tarantool/tarantool/pull/8002
Issue: https://github.com/tarantool/tarantool/issues/5688

 CMakeLists.txt                                |  9 +--
 Makefile.original                             |  7 +-
 src/CMakeLists.txt                            |  5 ++
 src/lj_tools_conf.h.in                        |  7 ++
 src/luajit.c                                  | 63 ++++++++++++++++--
 .../gh-5688-memprof-cli-flag.test.lua         | 64 +++++++++++++++++++
 tools/CMakeLists.txt                          |  2 +
 7 files changed, 145 insertions(+), 12 deletions(-)
 create mode 100644 src/lj_tools_conf.h.in
 create mode 100644 test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua

diff --git a/CMakeLists.txt b/CMakeLists.txt
index c870cce2..97d0d42f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -263,6 +263,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)
@@ -271,10 +276,6 @@ add_subdirectory(src)
 
 add_subdirectory(etc)
 
-# --- Tools --------------------------------------------------------------------
-
-add_subdirectory(tools)
-
 # --- Testing source tree ------------------------------------------------------
 
 # Auxiliary options for testing.
diff --git a/Makefile.original b/Makefile.original
index 0c92df9e..bb0ab73d 100644
--- a/Makefile.original
+++ b/Makefile.original
@@ -104,6 +104,7 @@ FILES_UTILSLIB= avl.lua bufread.lua symtab.lua
 FILES_MEMPROFLIB= parse.lua humanize.lua
 FILES_TOOLSLIB= memprof.lua
 FILE_TMEMPROF= luajit-parse-memprof
+FILE_TOOLSHEADER= lj_tools_conf.h.in
 
 ifeq (,$(findstring Windows,$(OS)))
   HOST_SYS:= $(shell uname -s)
@@ -193,7 +194,7 @@ clean:
 	$(RM) tools/$(FILE_TMEMPROF)
 	$(MAKE) -C src -f Makefile.original clean
 
-tools: tools/$(FILE_TMEMPROF)
+tools: tools/$(FILE_TMEMPROF) src/$(FILE_TOOLSHEADER)
 
 # FIXME: This is an ugly hack to manually configure an auxiliary
 # tools/luajit-parse-memprof. This file should go away in scope of
@@ -204,6 +205,10 @@ tools/$(FILE_TMEMPROF): src/luajit
 	     $@.in > $@
 	@chmod +x $@
 
+src/$(FILE_TOOLSHEADER):
+	@sed -e "s|@LUAJIT_TOOLS_DIR@|$(realpath tools)|" \
+	     $@.in > $@
+
 .PHONY: all install amalg clean tools
 
 ##############################################################################
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index dffc0a4d..17674a41 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -145,6 +145,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.
@@ -251,6 +253,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..9f9a2e49
--- /dev/null
+++ b/src/lj_tools_conf.h.in
@@ -0,0 +1,7 @@
+#ifndef LJ_TOOLS_CONF_H
+#define LJ_TOOLS_CONF_H
+
+#define TOOLS_PATH "@LUAJIT_TOOLS_DIR@/?.lua"
+#define PARSER_PATH "@LUAJIT_TOOLS_DIR@/memprof.lua"
+
+#endif
diff --git a/src/luajit.c b/src/luajit.c
index 1ca24301..bd9ae8f3 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"
+  "  -t[cmd]   Execute tool.\n"
   "  -E        Ignore environment variables.\n"
   "  --        Stop handling options.\n"
   "  -         Execute stdin and stop handling options.\n", stderr);
@@ -266,13 +269,9 @@ 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;
@@ -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 */
+  status = call_script(L, fname);
   return report(L, status);
 }
 
@@ -361,6 +370,15 @@ static int dojitcmd(lua_State *L, const char *cmd)
   return runcmdopt(L, opt ? opt+1 : opt);
 }
 
+static int dotoolcmd(lua_State *L, const char *cmd)
+{
+  if(strcmp(cmd, "m") == 0) {
+    const int status = call_script(L, PARSER_PATH);
+    return report(L, status);
+  }
+  return -1;
+}
+
 /* Optimization flags. */
 static int dojitopt(lua_State *L, const char *opt)
 {
@@ -390,6 +408,26 @@ static int dobytecode(lua_State *L, char **argv)
   return -1;
 }
 
+/*
+** On most Linux distros, it is the default value for the
+** maximum length of a string passed to `execve`.
+** However, there is no common value for other OSes, so
+** the size of 32 default memory pages is adopted.
+**/
+#define MAX_ENV_VAR 32 * 4096
+
+static int update_env_var(const char *name, const char *value)
+{
+  char env_buf[MAX_ENV_VAR] = "";
+  const char *env = getenv(name);
+  if (env == NULL) {
+    return setenv(name, value, 0);
+  } else {
+    strcpy(env_buf, env);
+    return setenv(name, strcat(env_buf, value), 0);
+  }
+}
+
 /* check that argument has no extra characters at the end */
 #define notail(x)	{if ((x)[2] != '\0') return -1;}
 
@@ -398,6 +436,7 @@ static int dobytecode(lua_State *L, char **argv)
 #define FLAGS_EXEC		4
 #define FLAGS_OPTION		8
 #define FLAGS_NOENV		16
+#define FLAGS_TOOL		32
 
 static int collectargs(char **argv, int *flags)
 {
@@ -419,6 +458,12 @@ static int collectargs(char **argv, int *flags)
       notail(argv[i]);
       *flags |= FLAGS_VERSION;
       break;
+    case 't':
+      *flags |= FLAGS_TOOL;
+      if (argv[i][2] == '\0') return -1;
+      if (argv[i + 1] == NULL) return -1;
+      update_env_var("LUA_PATH", TOOLS_PATH);
+      return i + 1;
     case 'e':
       *flags |= FLAGS_EXEC;
     case 'j':  /* LuaJIT extension */
@@ -474,6 +519,10 @@ static int runargs(lua_State *L, char **argv, int argn)
 	return 1;
       break;
       }
+    case 't': { /* Tarantool's fork extension. */
+      const char *cmd = argv[i] + 2;
+      return dotoolcmd(L, cmd) == LUA_OK;
+    }
     case 'O':  /* LuaJIT extension. */
       if (dojitopt(L, argv[i] + 2))
 	return 1;
@@ -535,7 +584,7 @@ static int pmain(lua_State *L)
   luaL_openlibs(L);
   lua_gc(L, LUA_GCRESTART, -1);
 
-  createargtable(L, argv, s->argc, argn);
+  createargtable(L, argv, s->argc, (flags & FLAGS_TOOL) ? argn - 1 : argn);
 
   if (!(flags & FLAGS_NOENV)) {
     s->status = handle_luainit(L);
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..ba2d0219
--- /dev/null
+++ b/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
@@ -0,0 +1,64 @@
+local utils = require('utils')
+
+-- XXX: The patch is for luajit only, and it doesn't
+-- work on Tarantool.
+-- luacheck: no global
+utils.skipcond(
+  (jit.arch ~= 'x86' and jit.arch ~= 'x64') or _TARANTOOL,
+  jit.arch..' architecture is NIY for memprof'
+)
+
+local tap = require('tap')
+
+local test = tap.test('gh-5688-memprof-cli-flag')
+test:plan(2)
+
+jit.off()
+jit.flush()
+
+local table_new = require 'table.new'
+
+local TMP_BINFILE = utils.profilename('memprofdata.tmp.bin')
+local BAD_PATH = utils.profilename('bad-path-tmp.bin')
+local EXECUTABLE = utils.luacmd(arg)
+
+local function default_payload()
+  -- Preallocate table to avoid table array part reallocations.
+  local _ = table_new(20, 0)
+
+  -- Want too see 20 objects here.
+  for i = 1, 20 do
+    -- Try to avoid crossing with "test" module objects.
+    _[i] = 'memprof-str-'..i
+  end
+
+  _ = nil
+  -- VMSTATE == GC, reported as INTERNAL.
+  collectgarbage()
+end
+
+local function generate_output(filename, payload)
+  -- Clean up all garbage to avoid pollution of free.
+  collectgarbage()
+
+  local res, err = misc.memprof.start(filename)
+  -- Should start succesfully.
+  assert(res, err)
+
+  payload()
+
+  res, err = misc.memprof.stop()
+  -- Should stop succesfully.
+  assert(res, err)
+end
+
+generate_output(TMP_BINFILE, default_payload)
+
+local errcode = os.execute(EXECUTABLE .. ' -tm ' .. BAD_PATH)
+test:ok(errcode ~= 0, 'binfile does not exist')
+
+errcode = os.execute(EXECUTABLE .. ' -tm ' .. TMP_BINFILE)
+test:ok(errcode == 0, 'memprof binfile parsing')
+
+os.remove(TMP_BINFILE)
+os.exit(test:check() and 0 or 1)
diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
index dd7ec6bd..e2e97b63 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
@@ -77,6 +78,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.38.1


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit v3] memprof: introduce cli flag to run dump parser
  2022-12-05 18:01 [Tarantool-patches] [PATCH luajit v3] memprof: introduce cli flag to run dump parser Maxim Kokryashkin via Tarantool-patches
@ 2023-02-07  7:23 ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 2+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-02-07  7:23 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

Hi, Maxim!

Thanks for the fixes!

I belive that this is the last iteration of the review, so LGTM,
after you'll fix some comments below.

On 05.12.22, 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 v3:
> - Fixed comments as per review by Sergey
> 
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-5688-cli-for-memprof-parse
> PR: https://github.com/tarantool/tarantool/pull/8002
> Issue: https://github.com/tarantool/tarantool/issues/5688
> 
>  CMakeLists.txt                                |  9 +--
>  Makefile.original                             |  7 +-
>  src/CMakeLists.txt                            |  5 ++
>  src/lj_tools_conf.h.in                        |  7 ++
>  src/luajit.c                                  | 63 ++++++++++++++++--
>  .../gh-5688-memprof-cli-flag.test.lua         | 64 +++++++++++++++++++
>  tools/CMakeLists.txt                          |  2 +
>  7 files changed, 145 insertions(+), 12 deletions(-)
>  create mode 100644 src/lj_tools_conf.h.in
>  create mode 100644 test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index c870cce2..97d0d42f 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt

<snipped>

> diff --git a/Makefile.original b/Makefile.original
> index 0c92df9e..bb0ab73d 100644
> --- a/Makefile.original
> +++ b/Makefile.original

<snipped>

> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index dffc0a4d..17674a41 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt

<snipped>

> diff --git a/src/lj_tools_conf.h.in b/src/lj_tools_conf.h.in
> new file mode 100644
> index 00000000..9f9a2e49
> --- /dev/null
> +++ b/src/lj_tools_conf.h.in

<snipped>

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

<snipped>

>  
> +/*
> +** On most Linux distros, it is the default value for the
> +** maximum length of a string passed to `execve`.
> +** However, there is no common value for other OSes, so
> +** the size of 32 default memory pages is adopted.
> +**/

Typo: s<**/><*/>

> +#define MAX_ENV_VAR 32 * 4096
> +
> +static int update_env_var(const char *name, const char *value)
> +{
> +  char env_buf[MAX_ENV_VAR] = "";
> +  const char *env = getenv(name);
> +  if (env == NULL) {
> +    return setenv(name, value, 0);
> +  } else {
> +    strcpy(env_buf, env);
> +    return setenv(name, strcat(env_buf, value), 0);

You shold use non-zero value here, if you want to rewrite the enviroment
variable.

| LUA_PATH=";;" src/luajit -tm /tmp/tmp_memprof.bin
| src/luajit: /home/burii/reviews/luajit/cli-flags/tools/memprof.lua:13: module 'utils.bufread' not found:
|         no field package.preload['utils.bufread']
| ...

But after the following patch it works fine:

===================================================================
diff --git a/src/luajit.c b/src/luajit.c
index bd9ae8f3..e40a4d30 100644
--- a/src/luajit.c
+++ b/src/luajit.c
@@ -424,7 +424,7 @@ static int update_env_var(const char *name, const char *value)
     return setenv(name, value, 0);
   } else {
     strcpy(env_buf, env);
-    return setenv(name, strcat(env_buf, value), 0);
+    return setenv(name, strcat(env_buf, value), 1);
   }
 }
 
===================================================================

| LUA_PATH="./?.lua;;" src/luajit -tm /tmp/tmp_memprof.bin
| ALLOCATIONS
| =(command line):1: 174 events   +6096 bytes     -0 bytes
| ...

Looks like a good testcase to add.

| +    return setenv(name, strcat(env_buf, value), 0);
Minor: I suggest to add a check length for total buffer length. (Yes,
it's highly unlikely, that someone has env var >= 128 KB, but we still
can use ENOMEM in such case.)
Feel free to ignore.

> +  }
> +}
> +
>  /* check that argument has no extra characters at the end */
>  #define notail(x)	{if ((x)[2] != '\0') return -1;}
>  
> @@ -398,6 +436,7 @@ static int dobytecode(lua_State *L, char **argv)
>  #define FLAGS_EXEC		4
>  #define FLAGS_OPTION		8
>  #define FLAGS_NOENV		16
> +#define FLAGS_TOOL		32
>  
>  static int collectargs(char **argv, int *flags)
>  {
> @@ -419,6 +458,12 @@ static int collectargs(char **argv, int *flags)
>        notail(argv[i]);
>        *flags |= FLAGS_VERSION;
>        break;
> +    case 't':
> +      *flags |= FLAGS_TOOL;
> +      if (argv[i][2] == '\0') return -1;
> +      if (argv[i + 1] == NULL) return -1;
> +      update_env_var("LUA_PATH", TOOLS_PATH);

Should we check the return value of the call here (EINVAL, or ENOMEM)?
If we don't worry about ENOMEM, feel free to ignore.

> +      return i + 1;
>      case 'e':
>        *flags |= FLAGS_EXEC;
>      case 'j':  /* LuaJIT extension */
> @@ -474,6 +519,10 @@ static int runargs(lua_State *L, char **argv, int argn)

<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..ba2d0219
> --- /dev/null
> +++ b/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
> @@ -0,0 +1,64 @@
> +local utils = require('utils')
> +
> +-- XXX: The patch is for luajit only, and it doesn't

Typo: s/luajit/LuaJIT/

> +-- work on Tarantool.
> +-- luacheck: no global

Nit: can we move luacheck comment one line below?
(Firstly don't get is it `utils` global)

> +utils.skipcond(
> +  (jit.arch ~= 'x86' and jit.arch ~= 'x64') or _TARANTOOL,
> +  jit.arch..' architecture is NIY for memprof'
> +)
> +
> +local tap = require('tap')
> +
> +local test = tap.test('gh-5688-memprof-cli-flag')
> +test:plan(2)
> +
> +jit.off()
> +jit.flush()
> +
> +local table_new = require 'table.new'
> +
> +local TMP_BINFILE = utils.profilename('memprofdata.tmp.bin')
> +local BAD_PATH = utils.profilename('bad-path-tmp.bin')
> +local EXECUTABLE = utils.luacmd(arg)
> +
> +local function default_payload()
> +  -- Preallocate table to avoid table array part reallocations.
> +  local _ = table_new(20, 0)

Minor: Should it be a constant as far as it is used twice?
Also, do we need this monkey business about table size and so on as far
as we don't check exactly values?

> +
> +  -- Want too see 20 objects here.
> +  for i = 1, 20 do
> +    -- Try to avoid crossing with "test" module objects.
> +    _[i] = 'memprof-str-'..i
> +  end
> +
> +  _ = nil
> +  -- VMSTATE == GC, reported as INTERNAL.
> +  collectgarbage()
> +end

<snipped>

> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index dd7ec6bd..e2e97b63 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt

<snipped>

> -- 
> 2.38.1
> 

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2023-02-07  7:27 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-05 18:01 [Tarantool-patches] [PATCH luajit v3] memprof: introduce cli flag to run dump parser Maxim Kokryashkin via Tarantool-patches
2023-02-07  7:23 ` Sergey Kaplun via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox