<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the fixes. LGTM now.</div><div> </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;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16846550441177828543_BODY">Hi, Maxim!<br>Thanks for the review!<br>Please consider my answers below.<br><br>On 15.05.23, Maxim Kokryashkin wrote:<br>><br>> Hi, Sergey!<br>> Thanks for the patch!<br>> Please consider my comments below.<br>> <br>> > <br>> >>This commit adds simple parser for traces to be analyzed in the test.<br>> >Typo: s/adds/adds a/<br><br>Fixed.<br><br>> >>For now nothing too fancy at all -- just start `jit.dump` to temporary<br>> >Typo: s/temporary/a temporary/<br><br>Fixed.<br><br>> >>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<br>> >Typo: s/if exists/if they exist/<br>> >Typo: s/it will be/it is/<br><br>Fixed, thanks!<br><br>> >>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><br><snipped><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<br>> >Two branches below are almost identical. Is it possible to avoid<br>> >duplication?<br><br>I suppose, no, especially since we may want to do some more work for<br>aborted traces (I just don't sure for now).<br><br>> >Side note: Also, I like it much more when switch-like constructions<br>> >in Lua are implemented with tables, because it seems to be cleaner.<br>> >Feel free to ignore.<br><br>Rewritten in this way, see the iterative patch below. Branch is<br>force-pushed.<br><br>===================================================================<br>diff --git a/test/tarantool-tests/utils/jit_parse.lua b/test/tarantool-tests/utils/jit_parse.lua<br>index fe8e0e08..7e8f879e 100644<br>--- a/test/tarantool-tests/utils/jit_parse.lua<br>+++ b/test/tarantool-tests/utils/jit_parse.lua<br>@@ -64,9 +64,9 @@ local function parse_mcode(trace, line)<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 header_handlers = {<br>+ start = function(ctx, line, trace_num)<br>+ local traces = ctx.traces<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>@@ -75,23 +75,27 @@ local function parse_trace_hdr(ctx, line, trace_num, status)<br> traces[trace_num] = trace<br> ctx.parsing_trace = trace_num<br> ctx.parsing = 'bc'<br>- elseif status == 'stop' then<br>+ end,<br>+ stop = function(ctx, line, trace_num)<br>+ local traces = ctx.traces<br> assert(ctx.parsing_trace == trace_num)<br> ctx.parsing_trace = nil<br> ctx.parsing = nil<br>- elseif status == 'abort' then<br>+ end,<br>+ abort = function(ctx, line, trace_num)<br>+ local traces = ctx.traces<br> assert(ctx.parsing_trace == trace_num)<br> ctx.parsing_trace = nil<br> ctx.parsing = nil<br> traces[trace_num] = nil<br>- elseif status == 'IR' then<br>+ end,<br>+ IR = function(ctx, line, trace_num)<br> ctx.parsing = 'IR'<br>- elseif status == 'mcode' then<br>+ end,<br>+ mcode = function(ctx, line, trace_num)<br> ctx.parsing = 'mcode'<br>- else<br>- error('Unknown trace status: ' .. status)<br>- end<br>-end<br>+ end,<br>+}<br> <br> local function parse_line(ctx, line)<br> if line == '' then<br>@@ -105,7 +109,11 @@ local function parse_line(ctx, line)<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>+ if (header_handlers[status]) then<br>+ header_handlers[status](ctx, line, tonumber(trace_num))<br>+ else<br>+ error('Unknown trace status: ' .. status)<br>+ end<br> return<br> end<br>===================================================================<br><br>> >>+ 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><br><snipped><br><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<br>> >I think we should perform error checking for that io operation.<br><br>It is done under the hood:<br>| $ src/luajit -e 'for _ in io.lines("unexisted") do end '<br>| src/luajit: (command line):1: bad argument #1 to 'lines' (unexisted: No such file or directory)<br>| stack traceback:<br>| [C]: in function 'lines'<br>| (command line):1: in main chunk<br>| [C]: at 0x55b73c281064<br><br>> >>+ 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'<br>> >Nit: What if either `A` or `H` is present in the flags? I believe, no<br>> >one is going to do such things, but anyway it is worth considering<br>> >as a thing to check. Feel free to ignore.<br><br>It is rewritten by the last flag (i.e. appended `T`).<br><br>> >>+ 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<br>> >>+<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<br>> >--<br>> >Best regards,<br>> >Maxim Kokryashkin<br>> > <br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>