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 EBDDB3A611B; Mon, 22 May 2023 10:06:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EBDDB3A611B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1684739198; bh=k9HdNRcfUojtbk8uYFSp7LBp4nqIa7n5AwFDVvpQ0Qw=; 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=u0hgrORrSrFb21rjf6h1bT3ta1MnhgintJIhT7KLk1MtLyNtV/Gk6zgOkt/GCK623 4KcW9e/nGhIJqLzmCmMSzqSUzdI3Nr7/IPE+MrAg0Pacu1sy3QpzWla6ShqgLmBt7i J/zPVwwYQE0Nog3It+yjeFMZEeRsLac4j74/bbuo= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 B6C8E3A611B for ; Mon, 22 May 2023 10:06:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B6C8E3A611B Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1q0zcp-0007Nc-Q9; Mon, 22 May 2023 10:06:36 +0300 Date: Mon, 22 May 2023 10:02:30 +0300 To: Sergey Bronnikov Message-ID: References: <44354d682d3cabf5a3ab1a268f900d7e6f981020.1683720396.git.skaplun@tarantool.org> <4959c9e3-e0e3-7d26-1a7f-836849cecf4c@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4959c9e3-e0e3-7d26-1a7f-836849cecf4c@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD921E8753A900160F17027C813E20C108255248BA8E4DCB833182A05F538085040D70AF9134BEAB98225FB476C16EA563AB8279816AB51331ECE7201011AF0CA03 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE77C1346FE4B18DC51EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637E8D1333770DC60CDEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BE5CCB53A13BC8DBA945145099B894FC7E87F854B87C2F424CC7F00164DA146DAFE8445B8C89999728AA50765F79006372255A60090D554F4389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8F7930B210FE8F7FEF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B08F9A42B2210255C75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A50FD58C9603BE73DD19A914E0B7DC3D7FEF8A8BF8A39B678DF87CCE6106E1FC07E67D4AC08A07B9B06A1CB4668A9CA5FACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF19CDC2AEE25B3B74E93455409927F94B16BE2B56A27A7B28BE4F837F81934D796E47D229C2E76ED444C44B1DB2C1FC291F030636A66293926424BD8D3F049B1BA8CAA110EC39F0D1 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojsZq9uZIzxfaRlsJGlQEhGg== X-DA7885C5: B48D987E8E02B3A25C7A025F916B066055824E84B1AD463F897E3CB9A5926789262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986B4F473C8CDB27EED08B484205909096350FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/2] test: add utility for parsing `jit.dump` 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, Sergey! Thanks for the review! On 16.05.23, Sergey Bronnikov wrote: > Hello, Sergey > > Thanks for the patch! See my three comments inline. > > On 5/10/23 15:34, Sergey Kaplun wrote: > > This commit adds simple parser for traces to be analyzed in the test. > > For now nothing too fancy at all -- just start `jit.dump` to temporary > > file and parse and remove this file when dump is stopped. The array > > with resulting traces is returned. > > > > For now, just find a single IR by pattern and return ref number and IR, > > if exists. More functionality may be added if it will be necessary for > > tests. > > --- > > test/tarantool-tests/utils/jit_parse.lua | 156 +++++++++++++++++++++++ > > 1 file changed, 156 insertions(+) > > create mode 100644 test/tarantool-tests/utils/jit_parse.lua > > > > diff --git a/test/tarantool-tests/utils/jit_parse.lua b/test/tarantool-tests/utils/jit_parse.lua > > new file mode 100644 > > index 00000000..fe8e0e08 > > --- /dev/null > > +++ b/test/tarantool-tests/utils/jit_parse.lua > > The main purpose of this script is using in tests. It could be dangerous > to leave it without tests at all, > > breakage in it could require time for debugging someday. From other side > I believe no one wants > > writing tests for test infrastructure. I suggest add at least a couple > of unit tests > > that will parse trace in plain text format and check that results are > equal to expected. Added the following unit test: =================================================================== diff --git a/test/tarantool-tests/unit-jit-parse.test.lua b/test/tarantool-tests/unit-jit-parse.test.lua new file mode 100644 index 00000000..536135c6 --- /dev/null +++ b/test/tarantool-tests/unit-jit-parse.test.lua @@ -0,0 +1,42 @@ +local tap = require('tap') +local test = tap.test('unit-jit-parse'):skipcond({ + ['Test requires JIT enabled'] = not jit.status(), +}) + +local jparse = require('utils.jit_parse') + +local expected_irs = { + -- Use IR numbers as keys, for more readable example. + -- `%d` is a workaround for GC64 | non-GC64 stack slot number. + [1] = 'int SLOAD #%d+%s+CI', + [2] = 'int ADD 0001 %+1', + [3] = 'int LE 0002 %+3', + [4] = '--- LOOP ------------', + [5] = 'int ADD 0002 %+1', + [6] = 'int LE 0005 %+3', + [7] = 'int PHI 0002 0005', +} +local N_TESTS = #expected_irs + +jit.opt.start('hotloop=1') + +test:plan(N_TESTS) + +-- Reset traces. +jit.flush() + +jparse.start('i') + +-- Loop to compile: +for _ = 1, 3 do end + +local traces = jparse.finish() +local loop_trace = traces[1] + +for irnum = 1, N_TESTS do + local ir_pattern = expected_irs[irnum] + local irref = loop_trace:has_ir(ir_pattern) + test:is(irref, irnum, 'find IR refernce by pattern: ' .. ir_pattern) +end + +os.exit(test:check() and 0 or 1) =================================================================== > > > @@ -0,0 +1,156 @@ > > + > > +M.start = function(flags) > > Comment with description of function's arguments will be useful here and > M.finish(). Added, the following comments as you suggested: =================================================================== diff --git a/test/tarantool-tests/utils/jit_parse.lua b/test/tarantool-tests/utils/jit_parse.lua index 2a9286c9..56267713 100644 --- a/test/tarantool-tests/utils/jit_parse.lua +++ b/test/tarantool-tests/utils/jit_parse.lua @@ -139,6 +139,8 @@ local function parse_jit_dump() return ctx.traces end +-- Start `jit.dump()` utility with the given flags, saving the +-- output in a temporary file. M.start = function(flags) assert(JDUMP_FILE == nil, 'jit_parse is already running') -- Always use plain text output. @@ -151,6 +153,9 @@ M.start = function(flags) jdump.start(flags, JDUMP_FILE) end +-- Stop `jit.dump()` utility parsing the output from the file and +-- remove this file after. +-- Returns an array of traces recorded during the run. M.finish = function() assert(JDUMP_FILE ~= nil, 'jit_parse is not running') jdump.off() =================================================================== > > For internal functions too, but it is up to you. I have no clear vision, how the bytecode or mcode, should be saved for futher analysis (just save them as is for now). So, I don't leave any comments near the internal functions. > > > + assert(JDUMP_FILE == nil, 'jit_parse is already running') > > + -- Always use plain text output. > > + flags = flags .. 'T' > > + JDUMP_FILE = os.tmpname() > > + jdump.start(flags, JDUMP_FILE) > > +end > > + > > +M.finish = function() > > + assert(JDUMP_FILE ~= nil, 'jit_parse is not running') > > + jdump.off() > > + local traces = parse_jit_dump() > > + os.remove(JDUMP_FILE) > > + JDUMP_FILE = nil > > + return traces > > +end > > + > > +-- Turn off compilation for these modules to avoid side effects. > > +jit.off(true, true) > > +jit.off(jdump.on, true) > > +jit.off(jdump.off, true) > > I would put these three lines on top of file or even disable/enable > compilation in M.start() and M.finish(). > > Without enabling compilation back you are mutating environment on using > this script and > > this can be unexpected for those who will use scripts. Yes, this is reasonable, fixed. But the first line, you mentioned disable JIT compilation for this particular module, so I just leave it as is. See the iterative patch below. =================================================================== diff --git a/test/tarantool-tests/utils/jit_parse.lua b/test/tarantool-tests/utils/jit_parse.lua index 7e8f879e..2a9286c9 100644 --- a/test/tarantool-tests/utils/jit_parse.lua +++ b/test/tarantool-tests/utils/jit_parse.lua @@ -143,6 +143,10 @@ M.start = function(flags) assert(JDUMP_FILE == nil, 'jit_parse is already running') -- Always use plain text output. flags = flags .. 'T' + -- Turn off traces compilation for `jit.dump` to avoid side + -- effects for the period of the testing. + jit.off(jdump.on, true) + jit.off(jdump.off, true) JDUMP_FILE = os.tmpname() jdump.start(flags, JDUMP_FILE) end @@ -150,15 +154,16 @@ end M.finish = function() assert(JDUMP_FILE ~= nil, 'jit_parse is not running') jdump.off() + -- Enable traces compilation for `jit.dump` back. + jit.on(jdump.on, true) + jit.on(jdump.off, true) local traces = parse_jit_dump() os.remove(JDUMP_FILE) JDUMP_FILE = nil return traces end --- Turn off compilation for these modules to avoid side effects. +-- Turn off compilation for the module to avoid side effects. jit.off(true, true) -jit.off(jdump.on, true) -jit.off(jdump.off, true) return M =================================================================== > > > + > > +return M -- Best regards, Sergey Kaplun