* [Tarantool-patches] [PATCH luajit v2] memprof: introduce cli flag to run dump parser
@ 2021-09-12 14:51 Maxim Kokryashkin via Tarantool-patches
2022-11-29 13:18 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 2+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2021-09-12 14:51 UTC (permalink / raw)
To: tarantool-patches, imun, skaplun
It is really unconvinient to use a standalone shell script to parse
memprof dump. That is why this commit intoroduces 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
---
GitHub branch:
https://github.com/tarantool/luajit/tree/fckxorg/gh-5688-cli-for-memprof-parse
Issue:
https://github.com/tarantool/tarantool/issues/5688
Also, I am not sure whether it is ok to do that[1] trick with `argn` or
not. It seems reasonable to run tools using the `runargs` method, but
then the `argn` should be adjusted in a such way, that the
`createargtable` will save parameters for the tool and 'runargs' will
execute the script itself.
[1]: https://github.com/tarantool/luajit/blob/8c074ebe7336a27f15484f1bb7eff757fa92c2f4/src/luajit.c#L565
CMakeLists.txt | 9 ++++----
src/CMakeLists.txt | 5 +++++
src/lj_tools_conf.h.in | 6 ++++++
src/luajit.c | 49 ++++++++++++++++++++++++++++++++----------
tools/CMakeLists.txt | 2 ++
5 files changed, 56 insertions(+), 15 deletions(-)
create mode 100644 src/lj_tools_conf.h.in
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 5348e043..74826095 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,10 +263,6 @@ add_subdirectory(src)
add_subdirectory(etc)
-# --- Tools --------------------------------------------------------------------
-
-add_subdirectory(tools)
-
# --- Testing source tree ------------------------------------------------------
# Auxiliary options for testing.
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
diff --git a/src/luajit.c b/src/luajit.c
index 1ca24301..525797a5 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,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 */
+ 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 status == LUA_OK;
+ }
+ return -1;
+}
+
/* Optimization flags. */
static int dojitopt(lua_State *L, const char *opt)
{
@@ -398,6 +416,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,14 +438,19 @@ 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 */
case 'l':
*flags |= FLAGS_OPTION;
if (argv[i][2] == '\0') {
- i++;
- if (argv[i] == NULL) return -1;
+ i++;
+ if (argv[i] == NULL) return -1;
}
break;
case 'O': break; /* LuaJIT extension */
@@ -474,6 +498,9 @@ static int runargs(lua_State *L, char **argv, int argn)
return 1;
break;
}
+ case 't' :
+ 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 +562,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/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.33.0
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [Tarantool-patches] [PATCH luajit v2] memprof: introduce cli flag to run dump parser
2021-09-12 14:51 [Tarantool-patches] [PATCH luajit v2] memprof: introduce cli flag to run dump parser Maxim Kokryashkin via Tarantool-patches
@ 2022-11-29 13:18 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 2+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-11-29 13:18 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Maxim!
Thanks for the patch!
Sorry, for such late review!
Please consider my comments below.
On 12.09.21, Maxim Kokryashkin wrote:
> It is really unconvinient to use a standalone shell script to parse
Typo: s/unconvinient/inconvenient/
> memprof dump. That is why this commit intoroduces a CLI flag for tools
Typo: s/intoroduces/introduces/
> to the LuaJIT, so now it is possible to parse memprof dump
> as simple as:
> ```
> luajit -tm memprof.bin
> ```
I don't see any error message in case of unexisted file or anything.
| $ src/luajit -tm /tmp/abracadabra
|
Also, it doesn't work for existing valid files:
| $ src/luajit -tm memprof.bin; echo $?
| 1
|
| $ tools/luajit-parse-memprof memprof.bin
| ALLOCATIONS
| INTERNAL: 948 events +32167 bytes -0 bytes
| ...
Am I doing smth wrong?
>
> Closes tarantool/tarantool#5688
> ---
> GitHub branch:
> https://github.com/tarantool/luajit/tree/fckxorg/gh-5688-cli-for-memprof-parse
>
> Issue:
> https://github.com/tarantool/tarantool/issues/5688
>
> Also, I am not sure whether it is ok to do that[1] trick with `argn` or
> not. It seems reasonable to run tools using the `runargs` method, but
> then the `argn` should be adjusted in a such way, that the
> `createargtable` will save parameters for the tool and 'runargs' will
> execute the script itself.
>
> [1]: https://github.com/tarantool/luajit/blob/8c074ebe7336a27f15484f1bb7eff757fa92c2f4/src/luajit.c#L565
>
> CMakeLists.txt | 9 ++++----
> src/CMakeLists.txt | 5 +++++
> src/lj_tools_conf.h.in | 6 ++++++
> src/luajit.c | 49 ++++++++++++++++++++++++++++++++----------
> tools/CMakeLists.txt | 2 ++
> 5 files changed, 56 insertions(+), 15 deletions(-)
Should we update original Makefile as well?
Also, it will be nice to add any tests for the new feature.
> create mode 100644 src/lj_tools_conf.h.in
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 5348e043..74826095 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -250,6 +250,11 @@ endif()
<snipped>
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 809aac68..d9debccf 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..366c3ec4
> --- /dev/null
> +++ b/src/lj_tools_conf.h.in
<snipped>
> diff --git a/src/luajit.c b/src/luajit.c
> index 1ca24301..525797a5 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,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);
I suppose, that this change is unnecessary.
> } 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 */
> + 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) {
Typo: trailing whytespace ------^
> + const int status = call_script(L, PARSER_PATH);
> + return status == LUA_OK;
> + }
> + return -1;
> +}
> +
> /* Optimization flags. */
> static int dojitopt(lua_State *L, const char *opt)
> {
> @@ -398,6 +416,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
Something wrong with indent here.
>
> static int collectargs(char **argv, int *flags)
> {
> @@ -419,14 +438,19 @@ 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 */
> case 'l':
> *flags |= FLAGS_OPTION;
> if (argv[i][2] == '\0') {
> - i++;
> - if (argv[i] == NULL) return -1;
> + i++;
> + if (argv[i] == NULL) return -1;
I suppose, that this change is unnecessary.
> }
> break;
> case 'O': break; /* LuaJIT extension */
> @@ -474,6 +498,9 @@ static int runargs(lua_State *L, char **argv, int argn)
> return 1;
> break;
> }
> + case 't' :
Minor: I suggest to add a comment that this is Tarantool's fork
extention.
> + 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 +562,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/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index 61830e44..84129153 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt
<snipped>
> --
> 2.33.0
>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-11-29 13:22 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-12 14:51 [Tarantool-patches] [PATCH luajit v2] memprof: introduce cli flag to run dump parser Maxim Kokryashkin via Tarantool-patches
2022-11-29 13:18 ` 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