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 9C63A5BE23B; Mon, 28 Aug 2023 14:22:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9C63A5BE23B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1693221749; bh=kXjKGMYDCyo5SJrW/WzfsXfeZ+1BUbuzpt69fSmYyBA=; 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=KYxOfIVoR+lxCAwOzEAnqpCkrsWy1iMcUqhrAShUGJcNwLkKev5OA7/iU5CCX95ag xTMwfa2rqtgSVscnWjXypuwIceMlebgmt4SuP35R06i+mcP+RUDbSiFY6A4R0LRoDa 1rpCGy29LlJi19+SJmsP0Wqi++wS4lhlVGp+qKx0= Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [95.163.41.77]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 975E542F2BE for ; Mon, 28 Aug 2023 14:22:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 975E542F2BE Received: by smtp36.i.mail.ru with esmtpa (envelope-from ) id 1qaaKA-002owc-2O; Mon, 28 Aug 2023 14:22:27 +0300 Date: Mon, 28 Aug 2023 14:17:42 +0300 To: Maxim Kokryashkin Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9954329A9C7AF96E9FF7E5CBDFDEDE4BE8F45FAF46EF4763D182A05F538085040C526BFEBC8DB088A6B9FC16D12D649A393630CC9C54DF518BB5A814782E99246 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE706EA9E10470DC775EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637BCF76D51F00B42068638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D87EA55FFDAF2491A8D581A8F9FFE784D0117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA1C5B227563B4AFA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751FF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C16F36C0CB6DCE122AD7EC71F1DB884274AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C342AF7BC6F74C2E4EBA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE75B51C8FB0C3E748C731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A510A1DB6BFD52A67D6DCA987C2E843E0AEC975A2F77FDD626F87CCE6106E1FC07E67D4AC08A07B9B067F1C1C3ABB44F3ACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF3B75371E1934E0FE090056C0D99CDA88E5A32A9D3E84CF3AA732EA00AE25C236A30287D0F4871F185088795BE901264802441C8D1BA15955409449FEAEA32D69A74DFFEFA5DC0E7F02C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojrC6BcuK2ROx3D4/118YTnw== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A7695C710EBD7632E63E6B9FC16D12D649A37958A49548C749F6DEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/2 v8] tools: add cli flag to run profile dump parsers 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 25.08.23, Maxim Kokryashkin wrote: > It is really inconvenient to use a standalone shell script to parse > binary dumps from profilers. That is why this commit introduces a > CLI flag for tools to the LuaJIT, so now it is possible to parse Typo? s/for tools to the/for tools in the/ > a memprof dump as simple as: > ``` > luajit -tm memprof.bin > luajit -tm --leak-only memprof.bin > ``` > > And the sysprof too: > ``` > luajit -ts sysprof.bin > luajit -ts --split sysprof.bin > ``` > > The `luajit-parse-memprof` and `luajit-parse-sysprof` are purged > as a result of this changes. Typo: s/this/these/ > > Closes tarantool/tarantool#5688 > --- > This patch requires additional changes for tarantool proxying, > that are done in the PR. > Makefile.original | 20 --- > src/luajit.c | 41 +++++- > .../gh-5688-tool-cli-flag.test.lua | 130 ++++++++++++++++++ > tools/CMakeLists.txt | 73 ---------- > tools/luajit-parse-memprof.in | 6 - > tools/luajit-parse-sysprof.in | 6 - > tools/memprof.lua | 4 +- > tools/sysprof.lua | 16 ++- > 8 files changed, 186 insertions(+), 110 deletions(-) > create mode 100644 test/tarantool-tests/gh-5688-tool-cli-flag.test.lua > delete mode 100755 tools/luajit-parse-memprof.in > delete mode 100644 tools/luajit-parse-sysprof.in > > diff --git a/Makefile.original b/Makefile.original > index 0c92df9e..10836f7d 100644 > --- a/Makefile.original > +++ b/Makefile.original > @@ -58,7 +58,6 @@ INSTALL_DYLIBSHORT2= libluajit-$(ABIVER).$(MAJVER).dylib > INSTALL_DYLIBNAME= libluajit-$(ABIVER).$(MAJVER).$(MINVER).$(RELVER).dylib > INSTALL_PCNAME= luajit.pc > INSTALL_TMEMPROFNAME= luajit-$(VERSION)-parse-memprof I suppose, that this should be dropped too. > -INSTALL_TMEMPROFSYMNAME= luajit-parse-memprof > > INSTALL_STATIC= $(INSTALL_LIB)/$(INSTALL_ANAME) > INSTALL_DYN= $(INSTALL_LIB)/$(INSTALL_SONAME) > @@ -68,7 +67,6 @@ INSTALL_T= $(INSTALL_BIN)/$(INSTALL_TNAME) > INSTALL_TSYM= $(INSTALL_BIN)/$(INSTALL_TSYMNAME) > INSTALL_PC= $(INSTALL_PKGCONFIG)/$(INSTALL_PCNAME) > INSTALL_TMEMPROF= $(INSTALL_BIN)/$(INSTALL_TMEMPROFNAME) As far as this one. > -INSTALL_TMEMPROFSYM= $(INSTALL_BIN)/$(INSTALL_TMEMPROFSYMNAME) > @@ -103,7 +99,6 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \ > FILES_UTILSLIB= avl.lua bufread.lua symtab.lua > FILES_MEMPROFLIB= parse.lua humanize.lua I noticed that this variable misses the recently introduced. Also, there are no libs to install via sysprof too. I suggest to fix this misbehaviour within the other patch (not even in this patchset). > FILES_TOOLSLIB= memprof.lua > -FILE_TMEMPROF= luajit-parse-memprof > > diff --git a/src/luajit.c b/src/luajit.c > index 1ca24301..e9ed89bc 100644 > --- a/src/luajit.c > +++ b/src/luajit.c > +static int runtoolcmd(lua_State *L, const char *tool_name) > +{ > + lua_getglobal(L, "require"); > + lua_pushstring(L, tool_name); > + if (lua_pcall(L, 1, 1, 0)) { > + const char *msg = lua_tostring(L, -1); > + if (msg && !strncmp(msg, "module ", 7)) > + l_message(progname, > + "unknown luaJIT command or tools not installed"); Nit: There is no need for new line here. Also, I strongly believe that we should report an error in the case, when there is another error reason. Just to avoid silent failures. > + return 1; > + } > + lua_getglobal(L, "arg"); > + return report(L, lua_pcall(L, 1, 1, 0)); > +} > + > diff --git a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua > new file mode 100644 > index 00000000..b91902fd > --- /dev/null > +++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua > + > +local function tool_test_case(case_name, cmd_set) > + test:test(case_name, function(subtest) > + subtest:plan(#cmd_set) > + > + for idx = 1, #cmd_set do > + local result = os.execute(cmd_set[idx].cmd .. SUPPRESS_OUTPUT) == 0 Here we got a problem: if sysprof has bad arguments, sysprof reports nothing -- just silently fails: | >>> LUA_PATH="tools/?.lua;;" src/luajit -ts /tmp/memprof.bin | 13:29:14 jobs:0 burii@root:~/reviews/luajit/cli-flags$[1] | >>> LUA_PATH="tools/?.lua;;" src/luajit -ts asdfl | 13:29:14 jobs:0 burii@root:~/reviews/luajit/cli-flags$[1] I suggest to check the error message too. > + subtest:ok(result == cmd_set[idx].expected, cmd_set[idx].cmd) > + end > + end) > +end > + > +tool_test_case('smoke', SMOKE_CMD_SET) > + > +generate_profiler_output(TMP_BINFILE_MEMPROF, memprof_payload, misc.memprof) > +tool_test_case('memprof parsing tool', MEMPROF_CMD_SET) > + > +local sysprof_opts = { mode = 'C', path = TMP_BINFILE_SYSPROF } > +generate_profiler_output(sysprof_opts, sysprof_payload, misc.sysprof) > +tool_test_case('sysprof parsing tool', SYSPROF_CMD_SET) > + > +os.remove(TMP_BINFILE_MEMPROF) > +os.remove(TMP_BINFILE_SYSPROF) > +test:done(true) > diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt > index dd7ec6bd..b7aeb472 100644 > --- a/tools/CMakeLists.txt > +++ b/tools/CMakeLists.txt > add_custom_target(tools-parse-memprof EXCLUDE_FROM_ALL DEPENDS > - luajit-parse-memprof > memprof/humanize.lua > memprof/parse.lua > memprof.lua is missing here. > @@ -66,27 +51,6 @@ else() > WORLD_READ > COMPONENT tools-parse-memprof > ) > add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS > - luajit-parse-sysprof > sysprof/parse.lua > sysprof/collapse.lua > sysprof.lua > @@ -148,27 +96,6 @@ else() > WORLD_READ > COMPONENT tools-parse-sysprof > ) > diff --git a/tools/luajit-parse-memprof.in b/tools/luajit-parse-memprof.in > deleted file mode 100755 > index 8867202a..00000000 > --- a/tools/luajit-parse-memprof.in > +++ /dev/null > diff --git a/tools/luajit-parse-sysprof.in b/tools/luajit-parse-sysprof.in > deleted file mode 100644 > index 2be25eb3..00000000 > --- a/tools/luajit-parse-sysprof.in > +++ /dev/null > diff --git a/tools/memprof.lua b/tools/memprof.lua > index cf66dd9e..8df1630a 100644 > --- a/tools/memprof.lua > +++ b/tools/memprof.lua > +-- XXX: When this script is used as a preloaded module by an > +-- application, it should return one function for correct parsing > +-- of command line flags like --leak-only and dumping profile > +-- info. > +local function dump_wrapped(...) > + return dump(parseargs(...)) > end > > -- FIXME: this script should be application-independent. > local args = {...} > if #args == 1 and args[1] == "sysprof" then > - return dump > + return dump_wrapped > else > - dump(parseargs(args)) > + dump_wrapped(args) Do we ever need this case since there is no direct call to it? I suppose that this FIXME and if/else branches may be deleted. Now we may always return the `dump_wrapped()` function. > end > -- > 2.41.0 > -- Best regards, Sergey Kaplun