Re: [Tarantool-patches] [PATCH luajit v3] memprof: introduce cli flag to run dump parser

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

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