

<HTML><BODY><div>Hi again!</div><div>It seems like I keep forgetting a single nit in every</div><div>review today.</div><div> </div><div>I am concerned about the `test/tarantool-tests/utils/jit_parse.lua`</div><div>path, since we already have the `test/tarantool-tests/utils.lua`</div><div>module. I think we should rename this directory to `tools` or</div><div>something like that.</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Понедельник, 15 мая 2023, 14:12 +03:00 от Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16841491280483250337_BODY"><div class="cl_152532"><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>Please consider my comments below.</div><div> </div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div id=""><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div id="style_16837223431084979031_BODY_mr_css_attr">This commit adds simple parser for traces to be analyzed in the test.</div></div></div></div></blockquote><div>Typo: s/adds/adds a/</div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>For now nothing too fancy at all -- just start `jit.dump` to temporary</div></div></div></div></blockquote><div>Typo: s/temporary/a temporary/</div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>file and parse and remove this file when dump is stopped. The array<br>with resulting traces is returned.<br><br>For now, just find a single IR by pattern and return ref number and IR,<br>if exists. More functionality may be added if it will be necessary for</div></div></div></div></blockquote><div>Typo: s/if exists/if they exist/</div><div>Typo: s/it will be/it is/</div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>tests.<br>---<br> test/tarantool-tests/utils/jit_parse.lua | 156 +++++++++++++++++++++++<br> 1 file changed, 156 insertions(+)<br> create mode 100644 test/tarantool-tests/utils/jit_parse.lua<br><br>diff --git a/test/tarantool-tests/utils/jit_parse.lua b/test/tarantool-tests/utils/jit_parse.lua<br>new file mode 100644<br>index 00000000..fe8e0e08<br>--- /dev/null<br>+++ b/test/tarantool-tests/utils/jit_parse.lua<br>@@ -0,0 +1,156 @@<br>+local jdump = require('jit.dump')<br>+<br>+local M = {}<br>+<br>+local trace_mt = {}<br>+trace_mt.__index = trace_mt<br>+<br>+-- Find and return the first IR ref and value matched the pattern<br>+-- for the trace.<br>+trace_mt.has_ir = function(trace, ir_pattern)<br>+ for i = 1, #trace.ir do<br>+ local ir = trace.ir[i]<br>+ if ir:match(ir_pattern) then<br>+ return i, ir<br>+ end<br>+ end<br>+end<br>+<br>+local function trace_new(n)<br>+ return setmetatable({<br>+ number = n,<br>+ parent = nil,<br>+ parent_exitno = nil,<br>+ start_loc = nil,<br>+ bc = {},<br>+ ir = {},<br>+ mcode = {},<br>+ snaps = {},<br>+ }, trace_mt)<br>+end<br>+<br>+local function parse_bc(trace, line)<br>+ trace.bc[#trace.bc + 1] = line<br>+end<br>+<br>+local function parse_snap(trace, n_snap, line)<br>+ assert(trace.snaps[n_snap] == nil)<br>+ trace[n_snap] = {ref = #trace.ir + 1, slots = line:match('%[(.*)%]$'),}<br>+end<br>+<br>+local function parse_ir(trace, line)<br>+ local n_snap = line:match('SNAP%s+#(%d+)')<br>+ if n_snap then<br>+ parse_snap(trace, n_snap, line)<br>+ return<br>+ end<br>+<br>+ local current_ins, ir = line:match('^(%d+)%s+(.*)$')<br>+ current_ins = tonumber(current_ins)<br>+ -- Insert NOP instruction hidden in IR dump.<br>+ if current_ins ~= #trace.ir + 1 then<br>+ trace.ir[#trace.ir + 1] = 'nil NOP'<br>+ end<br>+ assert(current_ins == #trace.ir + 1)<br>+ trace.ir[current_ins] = ir<br>+end<br>+<br>+local function parse_mcode(trace, line)<br>+ -- Skip loop label.<br>+ if line == '-> LOOP:' then<br>+ return<br>+ end<br>+ local addr, instruction = line:match('^(%w+)%s+(.*)$')<br>+ trace.mcode[#trace.mcode + 1] = {addr = addr, instruction = instruction,}<br>+end<br>+<br>+local function parse_trace_hdr(ctx, line, trace_num, status)<br>+ local traces = ctx.traces<br>+ if status == 'start' then<br>+ local trace = trace_new(trace_num)<br>+ trace.parent, trace.parent_exitno = line:match('start (%d+)/(%d+)')<br>+ -- XXX: Assume, that source line can't contain spaces.<br>+ -- For example, it's not "(command line)".<br>+ trace.start_loc = line:match(' ([^%s]+)$')<br>+ traces[trace_num] = trace<br>+ ctx.parsing_trace = trace_num<br>+ ctx.parsing = 'bc'<br>+ elseif status == 'stop' then<br>+ assert(ctx.parsing_trace == trace_num)<br>+ ctx.parsing_trace = nil<br>+ ctx.parsing = nil<br>+ elseif status == 'abort' then<br>+ assert(ctx.parsing_trace == trace_num)<br>+ ctx.parsing_trace = nil<br>+ ctx.parsing = nil<br>+ traces[trace_num] = nil</div></div></div></div></blockquote><div>Two branches below are almost identical. Is it possible to avoid</div><div>duplication?</div><div>Side note: Also, I like it much more when switch-like constructions</div><div>in Lua are implemented with tables, because it seems to be cleaner.</div><div>Feel free to ignore.</div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>+ elseif status == 'IR' then<br>+ ctx.parsing = 'IR'<br>+ elseif status == 'mcode' then<br>+ ctx.parsing = 'mcode'<br>+ else<br>+ error('Unknown trace status: ' .. status)<br>+ end<br>+end<br>+<br>+local function parse_line(ctx, line)<br>+ if line == '' then<br>+ return<br>+ end<br>+<br>+ if line:match('TRACE flush') then<br>+ ctx.traces = {}<br>+ return<br>+ end<br>+<br>+ local trace_num, status = line:match('TRACE (%d+) (%w+)')<br>+ if trace_num then<br>+ parse_trace_hdr(ctx, line, tonumber(trace_num), status)<br>+ return<br>+ end<br>+<br>+ assert(ctx.parsing_trace)<br>+<br>+ local trace = ctx.traces[ctx.parsing_trace]<br>+ if ctx.parsing == 'bc' then<br>+ parse_bc(trace, line)<br>+ elseif ctx.parsing == 'IR' then<br>+ parse_ir(trace, line)<br>+ elseif ctx.parsing == 'mcode' then<br>+ parse_mcode(trace, line)<br>+ end<br>+end<br>+<br>+local JDUMP_FILE<br>+<br>+local function parse_jit_dump()<br>+ local ctx = {traces = {}}<br>+ for line in io.lines(JDUMP_FILE) do</div></div></div></div></blockquote><div>I think we should perform error checking for that io operation.</div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>+ parse_line(ctx, line)<br>+ end<br>+ return ctx.traces<br>+end<br>+<br>+M.start = function(flags)<br>+ assert(JDUMP_FILE == nil, 'jit_parse is already running')<br>+ -- Always use plain text output.<br>+ flags = flags .. 'T'</div></div></div></div></blockquote><div>Nit: What if either `A` or `H` is present in the flags? I believe, no</div><div>one is going to do such things, but anyway it is worth considering</div><div>as a thing to check. Feel free to ignore.</div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>+ JDUMP_FILE = os.tmpname()<br>+ jdump.start(flags, JDUMP_FILE)<br>+end<br>+<br>+M.finish = function()<br>+ assert(JDUMP_FILE ~= nil, 'jit_parse is not running')<br>+ jdump.off()<br>+ local traces = parse_jit_dump()<br>+ os.remove(JDUMP_FILE)<br>+ JDUMP_FILE = nil<br>+ return traces<br>+end</div></div></div></div></blockquote><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>+<br>+-- Turn off compilation for these modules to avoid side effects.<br>+jit.off(true, true)<br>+jit.off(jdump.on, true)<br>+jit.off(jdump.off, true)<br>+<br>+return M<br>--<br>2.34.1</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div><div> </div></div></blockquote></div></div></div></div></div></blockquote><div> </div></BODY></HTML>