From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 38FCE6FC83; Fri, 20 Aug 2021 18:51:01 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 38FCE6FC83 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629474661; bh=1RJfXnPdDeXEKTHzMi4oR8bKI5UrTPA1wdMkF6YR5EU=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=qWPRmCZ48aKWf4NbxruUElSbyDRJX+6tufQA0Htr4oz7YrWhM04hpeUTWSi/FJcSv XQ3lUEaldLr6owgPkGJWTqn56DV0BpZ1DIxstW1lAji9Yh0mWvOHW81Dg/msOaacZd Hwg4cgJMTjAhHaqon87RVvSzdjoPHQ8MiLRf55Zk= Received: from smtp30.i.mail.ru (smtp30.i.mail.ru [94.100.177.90]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7D8396FC83 for ; Fri, 20 Aug 2021 18:50:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7D8396FC83 Received: by smtp30.i.mail.ru with esmtpa (envelope-from ) id 1mH6nK-0001Bf-Jg; Fri, 20 Aug 2021 18:50:59 +0300 Date: Fri, 20 Aug 2021 18:49:40 +0300 To: Maxim Kokryashkin Message-ID: References: <20210729115126.336490-1-m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210729115126.336490-1-m.kokryashkin@tarantool.org> X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9D5AC6413C25DCF08CC98B8FCC5CD86F3182A05F538085040FB7400C111EC3ACB5F8884FEEEDE5149CBAD8899E8D656E0E2063CF805F68D78 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75C385DEB91CEC222EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637FB2D77E6174520AE8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D863972DA20EC41656E97E0AB02C8E1DC9117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6A1DCCEB63E2F10FB089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A59B0387F3B8992484FD4D3F03E62A4175103CDA927135587FD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA758B25CD4253D1D611410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3475FE4AA98865E235D26B6DE2CC17E96107FE9859C783788068F678DD57C7EE2C453B81A8271B3CC51D7E09C32AA3244C9EED29C6F763C6A0E2E9D3EDC5540F657C0C08F7987826B9927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojZWOt9KMsrmd95nxnKeI3Zg== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB4421E26CAD8FABEA230ADFBC7E687FD6A23A23C9E1D0981CDF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] memprof: introduce a CLI flag to run dump parser X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > #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