Re: [Tarantool-patches] [PATCH luajit 1/2] test: add utility for parsing `jit.dump`

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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 @@

<snipped>

> > +
> > +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