Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser
@ 2023-07-10 11:24 Maksim Kokryashkin via Tarantool-patches
  2023-07-11 13:06 ` Sergey Bronnikov via Tarantool-patches
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Maksim Kokryashkin via Tarantool-patches @ 2023-07-10 11:24 UTC (permalink / raw)
  To: tarantool-patches, sergeyb, skaplun, m.kokryashkin

From: Maxim Kokryashkin <m.kokryashkin@tarantool.org>

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 v7:
- 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

 src/luajit.c                                  | 32 +++++++++-
 .../gh-5688-memprof-cli-flag.test.lua         | 58 +++++++++++++++++++
 2 files changed, 89 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..d335f32b 100644
--- a/src/luajit.c
+++ b/src/luajit.c
@@ -72,6 +72,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);
@@ -361,6 +362,25 @@ 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) {
+    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,
+          "tools.* modules not installed");
+      return 1;
+    }
+    lua_getglobal(L, "arg");
+    return report(L, lua_pcall(L, 1, 1, 0));
+  }
+  l_message(progname, "invalid tool command");
+  return -1;
+}
+
 /* Optimization flags. */
 static int dojitopt(lua_State *L, const char *opt)
 {
@@ -398,6 +418,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 +440,11 @@ 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;
+      return i + 1;
     case 'e':
       *flags |= FLAGS_EXEC;
     case 'j':  /* LuaJIT extension */
@@ -474,6 +500,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 +565,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..8361781f
--- /dev/null
+++ b/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
@@ -0,0 +1,58 @@
+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',
+-- XXX: The patch is for LuaJIT only, and it doesn't
+-- work on Tarantool.
+-- luacheck: no global
+  ['No memprof CLI option'] = _TARANTOOL,
+})
+
+test:plan(3)
+
+jit.off()
+jit.flush()
+
+local table_new = require('table.new')
+local utils = require('utils')
+
+local TMP_BINFILE = utils.tools.profilename('memprofdata.tmp.bin')
+local BAD_PATH = utils.tools.profilename('bad-path-tmp.bin')
+local EXECUTABLE = utils.exec.luacmd(arg)
+local TABLE_SIZE = 20
+
+local function default_payload()
+  local _ = table_new(TABLE_SIZE, 0)
+   _ = nil
+  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 .. ' -tinvalid ' .. TMP_BINFILE)
+test:ok(errcode ~= 0, 'invalid tool flag')
+
+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)
--
2.39.2 (Apple Git-143)


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

* Re: [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser
  2023-07-10 11:24 [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser Maksim Kokryashkin via Tarantool-patches
@ 2023-07-11 13:06 ` Sergey Bronnikov via Tarantool-patches
  2023-07-14 12:41   ` Maxim Kokryashkin via Tarantool-patches
  2023-07-15 11:35 ` Sergey Kaplun via Tarantool-patches
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-07-11 13:06 UTC (permalink / raw)
  To: Maksim Kokryashkin, tarantool-patches, skaplun, m.kokryashkin

Hello, Max!

Thanks for the patch! See my comments.


Sergey

On 7/10/23 14:24, Maksim Kokryashkin wrote:
> From: Maxim Kokryashkin <m.kokryashkin@tarantool.org>
>
> 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 v7:
> - 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
>
>   src/luajit.c                                  | 32 +++++++++-
>   .../gh-5688-memprof-cli-flag.test.lua         | 58 +++++++++++++++++++
>   2 files changed, 89 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..d335f32b 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c
> @@ -72,6 +72,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"

According to usage cmd after -t option is optional. However without cmd 
luajit prints usage

that hints about incorrect usage:


[0] ~/sources/MRG/tarantool/third_party/luajit$ ./src/luajit -t
usage: ./src/luajit [options]... [script [args]...].
Available options are:
   -e chunk  Execute string 'chunk'.
   -l name   Require library 'name'.
   -b ...    Save or list bytecode.
   -j cmd    Perform LuaJIT control command.
   -O[opt]   Control LuaJIT optimizations.
   -i        Enter interactive mode after executing 'script'.
   -v        Show version information.
   -t[cmd]   Execute tool.
   -E        Ignore environment variables.
   --        Stop handling options.
   -         Execute stdin and stop handling options.


<snipped>
> +end
> +
> +generate_output(TMP_BINFILE, default_payload)
> +
> +local errcode = os.execute(EXECUTABLE .. ' -tinvalid ' .. TMP_BINFILE)
> +test:ok(errcode ~= 0, 'invalid tool flag')
> +
> +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')
Nit: probably it is worth adding  checks for output.
> +
> +os.remove(TMP_BINFILE)
> +os.exit(test:check() and 0 or 1)
> --
> 2.39.2 (Apple Git-143)
>

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

* Re: [Tarantool-patches]  [PATCH luajit v7] memprof: introduce cli flag to run dump parser
  2023-07-11 13:06 ` Sergey Bronnikov via Tarantool-patches
@ 2023-07-14 12:41   ` Maxim Kokryashkin via Tarantool-patches
  2023-07-28 15:30     ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 1 reply; 11+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-07-14 12:41 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: Maksim Kokryashkin, tarantool-patches

[-- Attachment #1: Type: text/plain, Size: 3656 bytes --]


Hi! Thanks for the review.
 
> 
>>Hello, Max!
>>
>>Thanks for the patch! See my comments.
>>
>>
>>Sergey
>>
>>On 7/10/23 14:24, Maksim Kokryashkin wrote:
>>> From: Maxim Kokryashkin < m.kokryashkin@tarantool.org >
>>>
>>> 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 v7:
>>> - 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
>>>
>>> src/luajit.c | 32 +++++++++-
>>> .../gh-5688-memprof-cli-flag.test.lua | 58 +++++++++++++++++++
>>> 2 files changed, 89 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..d335f32b 100644
>>> --- a/src/luajit.c
>>> +++ b/src/luajit.c
>>> @@ -72,6 +72,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"
>>
>>According to usage cmd after -t option is optional. However without cmd
>>luajit prints usage
>>
>>that hints about incorrect usage:
>>
>>
>>[0] ~/sources/MRG/tarantool/third_party/luajit$ ./src/luajit -t
>>usage: ./src/luajit [options]... [script [args]...].
>>Available options are:
>>   -e chunk  Execute string 'chunk'.
>>   -l name   Require library 'name'.
>>   -b ...    Save or list bytecode.
>>   -j cmd    Perform LuaJIT control command.
>>   -O[opt]   Control LuaJIT optimizations.
>>   -i        Enter interactive mode after executing 'script'.
>>   -v        Show version information.
>>   -t[cmd]   Execute tool.
>>   -E        Ignore environment variables.
>>   --        Stop handling options.
>>   -         Execute stdin and stop handling options.
>Fixed! Branch is force-pushed, here is the diff:
>=========================================================
>diff --git a/src/luajit.c b/src/luajit.c
>index d335f32b..c16138f0 100644
>--- a/src/luajit.c
>+++ b/src/luajit.c
>@@ -72,7 +72,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"
>+  "  -t(cmd)   Execute tool.\n"
>   "  -E        Ignore environment variables.\n"
>   "  --        Stop handling options.\n"
>   "  -         Execute stdin and stop handling options.\n", stderr);
>=========================================================
>>
>><snipped>
>>> +end
>>> +
>>> +generate_output(TMP_BINFILE, default_payload)
>>> +
>>> +local errcode = os.execute(EXECUTABLE .. ' -tinvalid ' .. TMP_BINFILE)
>>> +test:ok(errcode ~= 0, 'invalid tool flag')
>>> +
>>> +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')
>>Nit: probably it is worth adding  checks for output.
>>> +
>>> +os.remove(TMP_BINFILE)
>>> +os.exit(test:check() and 0 or 1)
>>> --
>>> 2.39.2 (Apple Git-143)
>>>
>--
>Best regards,
>Maxim Kokryashkin
> 

[-- Attachment #2: Type: text/html, Size: 5361 bytes --]

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

* Re: [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser
  2023-07-10 11:24 [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser Maksim Kokryashkin via Tarantool-patches
  2023-07-11 13:06 ` Sergey Bronnikov via Tarantool-patches
@ 2023-07-15 11:35 ` Sergey Kaplun via Tarantool-patches
  2023-08-01 15:45 ` Igor Munkin via Tarantool-patches
  2023-08-02  7:23 ` Igor Munkin via Tarantool-patches
  3 siblings, 0 replies; 11+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-07-15 11:35 UTC (permalink / raw)
  To: Maksim Kokryashkin; +Cc: tarantool-patches

Hi, Maxim!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

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

* Re: [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser
  2023-07-14 12:41   ` Maxim Kokryashkin via Tarantool-patches
@ 2023-07-28 15:30     ` Sergey Bronnikov via Tarantool-patches
  2023-07-29 16:24       ` Igor Munkin via Tarantool-patches
  0 siblings, 1 reply; 11+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-07-28 15:30 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: Maksim Kokryashkin, tarantool-patches

[-- Attachment #1: Type: text/plain, Size: 4541 bytes --]

Max, thanks for the <s>cake</s> patches! LGTM


On 7/14/23 15:41, Maxim Kokryashkin wrote:
> Hi! Thanks for the review.
>
>         Hello, Max!
>
>         Thanks for the patch! See my comments.
>
>
>         Sergey
>
>         On 7/10/23 14:24, Maksim Kokryashkin wrote:
>         > From: Maxim Kokryashkin <m.kokryashkin@tarantool.org
>         </compose?To=m.kokryashkin@tarantool.org>>
>         >
>         > 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 v7:
>         > - 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
>         >
>         > src/luajit.c | 32 +++++++++-
>         > .../gh-5688-memprof-cli-flag.test.lua | 58 +++++++++++++++++++
>         > 2 files changed, 89 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..d335f32b 100644
>         > --- a/src/luajit.c
>         > +++ b/src/luajit.c
>         > @@ -72,6 +72,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"
>
>         According to usage cmd after -t option is optional. However
>         without cmd
>         luajit prints usage
>
>         that hints about incorrect usage:
>
>
>         [0] ~/sources/MRG/tarantool/third_party/luajit$ ./src/luajit -t
>         usage: ./src/luajit [options]... [script [args]...].
>         Available options are:
>            -e chunk  Execute string 'chunk'.
>            -l name   Require library 'name'.
>            -b ...    Save or list bytecode.
>            -j cmd    Perform LuaJIT control command.
>            -O[opt]   Control LuaJIT optimizations.
>            -i        Enter interactive mode after executing 'script'.
>            -v        Show version information.
>            -t[cmd]   Execute tool.
>            -E        Ignore environment variables.
>            --        Stop handling options.
>            -         Execute stdin and stop handling options.
>
>     Fixed! Branch is force-pushed, here is the diff:
>     =========================================================
>     diff --git a/src/luajit.c b/src/luajit.c
>     index d335f32b..c16138f0 100644
>     --- a/src/luajit.c
>     +++ b/src/luajit.c
>     @@ -72,7 +72,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"
>     +  "  -t(cmd)   Execute tool.\n"
>        "  -E        Ignore environment variables.\n"
>        "  --        Stop handling options.\n"
>        "  -         Execute stdin and stop handling options.\n", stderr);
>     =========================================================
>
>
>         <snipped>
>         > +end
>         > +
>         > +generate_output(TMP_BINFILE, default_payload)
>         > +
>         > +local errcode = os.execute(EXECUTABLE .. ' -tinvalid ' ..
>         TMP_BINFILE)
>         > +test:ok(errcode ~= 0, 'invalid tool flag')
>         > +
>         > +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')
>         Nit: probably it is worth adding  checks for output.
>         > +
>         > +os.remove(TMP_BINFILE)
>         > +os.exit(test:check() and 0 or 1)
>         > --
>         > 2.39.2 (Apple Git-143)
>         >
>
>     --
>     Best regards,
>     Maxim Kokryashkin
>

[-- Attachment #2: Type: text/html, Size: 8348 bytes --]

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

* Re: [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser
  2023-07-28 15:30     ` Sergey Bronnikov via Tarantool-patches
@ 2023-07-29 16:24       ` Igor Munkin via Tarantool-patches
  0 siblings, 0 replies; 11+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-07-29 16:24 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: Maksim Kokryashkin, tarantool-patches

Sergey,

On 28.07.23, Sergey Bronnikov via Tarantool-patches wrote:
> Max, thanks for the <s>cake</s> patches! LGTM

JFYI, the cake is a lie[1].

> 

[1]: https://en.wikipedia.org/wiki/The_cake_is_a_lie

-- 
Best regards,
IM

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

* Re: [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser
  2023-07-10 11:24 [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser Maksim Kokryashkin via Tarantool-patches
  2023-07-11 13:06 ` Sergey Bronnikov via Tarantool-patches
  2023-07-15 11:35 ` Sergey Kaplun via Tarantool-patches
@ 2023-08-01 15:45 ` Igor Munkin via Tarantool-patches
  2023-08-02  7:23 ` Igor Munkin via Tarantool-patches
  3 siblings, 0 replies; 11+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-08-01 15:45 UTC (permalink / raw)
  To: Maksim Kokryashkin; +Cc: tarantool-patches

Max,

Thanks for the changes! It would be nice to drop luajit-parse-memprof.in
with the respective CMake parts, since I see no purpose for this now.

-- 
Best regards,
IM

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

* Re: [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser
  2023-07-10 11:24 [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser Maksim Kokryashkin via Tarantool-patches
                   ` (2 preceding siblings ...)
  2023-08-01 15:45 ` Igor Munkin via Tarantool-patches
@ 2023-08-02  7:23 ` Igor Munkin via Tarantool-patches
  2023-08-02  7:59   ` Maxim Kokryashkin via Tarantool-patches
  3 siblings, 1 reply; 11+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-08-02  7:23 UTC (permalink / raw)
  To: Maksim Kokryashkin; +Cc: tarantool-patches

Max,

We've discussed with Sergey K. how to run -tm with --leak-only today.
Fortunately, it works fine in LuaJIT, however, I'm afraid such flag
handling in incompatible in Tarantool. Hence, I suggest to glue this
flag with comma to -tm, like jdump does[1]. Thoughts?

[1]: https://github.com/tarantool/luajit/blob/tarantool/master/src/jit/dump.lua#L18

-- 
Best regards,
IM

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

* Re: [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser
  2023-08-02  7:23 ` Igor Munkin via Tarantool-patches
@ 2023-08-02  7:59   ` Maxim Kokryashkin via Tarantool-patches
  2023-08-02  8:12     ` Igor Munkin via Tarantool-patches
  0 siblings, 1 reply; 11+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-08-02  7:59 UTC (permalink / raw)
  To: Igor Munkin; +Cc: Maksim Kokryashkin, tarantool-patches

Hi, Igor!

Well, I see no obstacles with flag handling in Tarantool.
For instance, the -j flag proxy can do exactly what you want.

| review/tarantool/build  fckxorg/gh-5688-cli-for-memprof-parse ✔                                                                                                                                   239d16h
| ▶ ./src/tarantool -j on -e 'print(jit.status())'
| true	fold	cse	dce	fwd	dse	narrow	loop	abc	sink	fuse
|
| review/tarantool/build  fckxorg/gh-5688-cli-for-memprof-parse ✔                                                                                                                                   239d16h
| ▶ ./src/tarantool -j off -e 'print(jit.status())'
| false	fold	cse	dce	fwd	dse	narrow	loop	abc	sink	fuse

Tarantool's flag handling is not pleasant to say the least,
but we should try to do the same as with -j.

Best regards,
Maxim Kokryashkin


On Wed, Aug 02, 2023 at 07:23:03AM +0000, Igor Munkin wrote:
> Max,
> 
> We've discussed with Sergey K. how to run -tm with --leak-only today.
> Fortunately, it works fine in LuaJIT, however, I'm afraid such flag
> handling in incompatible in Tarantool. Hence, I suggest to glue this
> flag with comma to -tm, like jdump does[1]. Thoughts?
> 
> [1]: https://github.com/tarantool/luajit/blob/tarantool/master/src/jit/dump.lua#L18
> 
> -- 
> Best regards,
> IM

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

* Re: [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser
  2023-08-02  7:59   ` Maxim Kokryashkin via Tarantool-patches
@ 2023-08-02  8:12     ` Igor Munkin via Tarantool-patches
  2023-08-02  8:32       ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 1 reply; 11+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-08-02  8:12 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: Maksim Kokryashkin, tarantool-patches

Max,

On 02.08.23, Maxim Kokryashkin wrote:
> Hi, Igor!
> 
> Well, I see no obstacles with flag handling in Tarantool.
> For instance, the -j flag proxy can do exactly what you want.
> 
> | review/tarantool/build  fckxorg/gh-5688-cli-for-memprof-parse ✔                                                                                                                                   239d16h
> | ▶ ./src/tarantool -j on -e 'print(jit.status())'
> | true	fold	cse	dce	fwd	dse	narrow	loop	abc	sink	fuse
> |
> | review/tarantool/build  fckxorg/gh-5688-cli-for-memprof-parse ✔                                                                                                                                   239d16h
> | ▶ ./src/tarantool -j off -e 'print(jit.status())'
> | false	fold	cse	dce	fwd	dse	narrow	loop	abc	sink	fuse
> 

My bad, I'm concerted rather about this difference and its effect in
Tarantool flags processing:
| $ ./luajit -bl -e 'print("qq")'
| -- BYTECODE -- "print("qq")":0-1
| 0001    GGET     0   0      ; "print"
| 0002    KSTR     1   1      ; "qq"
| 0003    CALL     0   1   2
| 0004    RET0     0   1
| 
| $ cd ../src/tools
| $ ../src/luajit -tm -e 'print("qq")'
| luajit-parse-memprof.lua: ERROR: unrecognized option `-e'. Try `--help'.
| 

In other words, I wonder
* whether flags in Tarantool are position independent (i.e. the result
  of -j + -e combination equals to -e + -j);
* how --leak-only will be handled in this case.

BTW, I have a thought regarding introducing kinda "mode" for memprof, so
we can use it like -tm=leak-only (as an alternative to the original
proposal in the first message).

Besides, I see no tests for --leak-only (have no idea, why everyone
missed this on review). Could you add them?

> Tarantool's flag handling is not pleasant to say the least,
> but we should try to do the same as with -j.
> 
> Best regards,
> Maxim Kokryashkin
> 
> 
> On Wed, Aug 02, 2023 at 07:23:03AM +0000, Igor Munkin wrote:
> > Max,
> > 
> > We've discussed with Sergey K. how to run -tm with --leak-only today.
> > Fortunately, it works fine in LuaJIT, however, I'm afraid such flag
> > handling in incompatible in Tarantool. Hence, I suggest to glue this
> > flag with comma to -tm, like jdump does[1]. Thoughts?
> > 
> > [1]: https://github.com/tarantool/luajit/blob/tarantool/master/src/jit/dump.lua#L18
> > 
> > -- 
> > Best regards,
> > IM

-- 
Best regards,
IM

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

* Re: [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser
  2023-08-02  8:12     ` Igor Munkin via Tarantool-patches
@ 2023-08-02  8:32       ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 0 replies; 11+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-08-02  8:32 UTC (permalink / raw)
  To: Igor Munkin; +Cc: Maksim Kokryashkin, tarantool-patches

Igor,

On Wed, Aug 02, 2023 at 08:12:50AM +0000, Igor Munkin wrote:
> Max,
> 
> On 02.08.23, Maxim Kokryashkin wrote:
> > Hi, Igor!
> > 
> > Well, I see no obstacles with flag handling in Tarantool.
> > For instance, the -j flag proxy can do exactly what you want.
> > 
> > | review/tarantool/build  fckxorg/gh-5688-cli-for-memprof-parse ✔                                                                                                                                   239d16h
> > | ▶ ./src/tarantool -j on -e 'print(jit.status())'
> > | true	fold	cse	dce	fwd	dse	narrow	loop	abc	sink	fuse
> > |
> > | review/tarantool/build  fckxorg/gh-5688-cli-for-memprof-parse ✔                                                                                                                                   239d16h
> > | ▶ ./src/tarantool -j off -e 'print(jit.status())'
> > | false	fold	cse	dce	fwd	dse	narrow	loop	abc	sink	fuse
> > 
> 
> My bad, I'm concerted rather about this difference and its effect in
> Tarantool flags processing:
> | $ ./luajit -bl -e 'print("qq")'
> | -- BYTECODE -- "print("qq")":0-1
> | 0001    GGET     0   0      ; "print"
> | 0002    KSTR     1   1      ; "qq"
> | 0003    CALL     0   1   2
> | 0004    RET0     0   1
> | 
> | $ cd ../src/tools
> | $ ../src/luajit -tm -e 'print("qq")'
> | luajit-parse-memprof.lua: ERROR: unrecognized option `-e'. Try `--help'.
> | 
> 
> In other words, I wonder
> * whether flags in Tarantool are position independent (i.e. the result
>   of -j + -e combination equals to -e + -j);

| review/tarantool/build  fckxorg/gh-5688-cli-for-memprof-parse ✔                                                                                                                                   239d17h
| ▶ ./src/tarantool -e 'print(jit.status())' -jon
| false	fold	cse	dce	fwd	dse	narrow	loop	abc	sink	fuse
|
| review/tarantool/build  fckxorg/gh-5688-cli-for-memprof-parse ✔                                                                                                                                   239d17h
| ▶ ./src/tarantool -jon  -e 'print(jit.status())'
| true	fold	cse	dce	fwd	dse	narrow	loop	abc	sink	fuse
So no, they are not position-independent.

> * how --leak-only will be handled in this case.
I can just pass further args after the -tm right to the memprof parser module then.

> 
> BTW, I have a thought regarding introducing kinda "mode" for memprof, so
> we can use it like -tm=leak-only (as an alternative to the original
> proposal in the first message).
> 
> Besides, I see no tests for --leak-only (have no idea, why everyone
> missed this on review). Could you add them?
Yep, sure.
> 
> > Tarantool's flag handling is not pleasant to say the least,
> > but we should try to do the same as with -j.
> > 
> > Best regards,
> > Maxim Kokryashkin
> > 
> > 
> > On Wed, Aug 02, 2023 at 07:23:03AM +0000, Igor Munkin wrote:
> > > Max,
> > > 
> > > We've discussed with Sergey K. how to run -tm with --leak-only today.
> > > Fortunately, it works fine in LuaJIT, however, I'm afraid such flag
> > > handling in incompatible in Tarantool. Hence, I suggest to glue this
> > > flag with comma to -tm, like jdump does[1]. Thoughts?
> > > 
> > > [1]: https://github.com/tarantool/luajit/blob/tarantool/master/src/jit/dump.lua#L18
> > > 
> > > -- 
> > > Best regards,
> > > IM
> 
> -- 
> Best regards,
> IM

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

end of thread, other threads:[~2023-08-02  8:33 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-10 11:24 [Tarantool-patches] [PATCH luajit v7] memprof: introduce cli flag to run dump parser Maksim Kokryashkin via Tarantool-patches
2023-07-11 13:06 ` Sergey Bronnikov via Tarantool-patches
2023-07-14 12:41   ` Maxim Kokryashkin via Tarantool-patches
2023-07-28 15:30     ` Sergey Bronnikov via Tarantool-patches
2023-07-29 16:24       ` Igor Munkin via Tarantool-patches
2023-07-15 11:35 ` Sergey Kaplun via Tarantool-patches
2023-08-01 15:45 ` Igor Munkin via Tarantool-patches
2023-08-02  7:23 ` Igor Munkin via Tarantool-patches
2023-08-02  7:59   ` Maxim Kokryashkin via Tarantool-patches
2023-08-02  8:12     ` Igor Munkin via Tarantool-patches
2023-08-02  8:32       ` Maxim Kokryashkin 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