Re: [Tarantool-patches] [PATCH luajit] Actually implement maxirconst trace limit.

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

Hi, Sergos!

Thanks for the review!

On 28.01.22, sergos wrote:
> Hi!
> 
> I reviewed the one at https://github.com/tarantool/luajit/commit/2cdd5ac0367629d2f37489dd842d9a6b068cf12c <https://github.com/tarantool/luajit/commit/2cdd5ac0367629d2f37489dd842d9a6b068cf12c>
> 
> Overall is LGTM, I would like to update the 2nd test message to something like
> ’trace should not appear due to maxirconst limit’

Fixed, see the iterative patch below.
Branch is force-pushed.

===================================================================
diff --git a/test/tarantool-tests/lj-430-maxirconst.test.lua b/test/tarantool-tests/lj-430-maxirconst.test.lua
index 79553ecb..10de2520 100644
--- a/test/tarantool-tests/lj-430-maxirconst.test.lua
+++ b/test/tarantool-tests/lj-430-maxirconst.test.lua
@@ -38,6 +38,6 @@ irconst4()
 irconst4()
 jit.off()
 test:ok(ntrace_old == misc.getmetrics().jit_trace_num,
-        'trace number is the same')
+        'trace should not appear due to maxirconst limit')
 
 os.exit(test:check() and 0 or 1)
===================================================================

> 
> Sergos
> 
> > On 28 Jan 2022, at 15:35, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > Igor,
> > 
> > On 28.01.22, Igor Munkin wrote:
> >> Sergey,
> >> 
> >> On 28.01.22, Sergey Kaplun wrote:
> >>> Igor,
> >>> 
> >>> Thanks for the review!
> >>> 
> >> 
> >> <snipped>
> >> 
> >>>>> 
> >>>>> Issue: https://github.com/LuaJIT/LuaJIT/issues/430
> >>>>> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-430-maxirconst-actually-implement-full-ci
> >>>>> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/lj-430-maxirconst-actually-implement-full-ci
> >>>>> 
> >>>>> src/lj_record.c                               |  5 ++-
> >>>>> .../lj-430-maxirconst.test.lua                | 43 +++++++++++++++++++
> >>>>> 2 files changed, 46 insertions(+), 2 deletions(-)
> >>>>> create mode 100644 test/tarantool-tests/lj-430-maxirconst.test.lua
> >>>>> 
> >>>> 
> >>>> <snipped>
> >>>> 
> >>>>> diff --git a/test/tarantool-tests/lj-430-maxirconst.test.lua b/test/tarantool-tests/lj-430-maxirconst.test.lua
> >>>>> new file mode 100644
> >>>>> index 00000000..1829b37d
> >>>>> --- /dev/null
> >>>>> +++ b/test/tarantool-tests/lj-430-maxirconst.test.lua
> >>>>> @@ -0,0 +1,43 @@
> >>>>> +-- XXX: avoid any other traces compilation due to hotcount
> >>>>> +-- collisions for predictible results.
> >> 
> >> <snipped>
> >> 
> >>>>> +jit.off()
> >>>>> +jit.flush()
> >>>> 
> >>>> Minor: I'd rather move this part closer to 'jit.opt.start' to save the
> >>>> test structure closer to the other test chunks. Feel free to ignore.
> >>> 
> >>> I really want to exclude __any__ JIT work here to avoid false-positive
> >>> hotcount (was precendents during writing this test :)). Ignoring.
> >> 
> >> OK, got it.
> >> 
> >>> 
> >> 
> >> <snipped>
> >> 
> >>>>> +test:ok(ntrace_old + 1 == misc.getmetrics().jit_trace_num,
> >>>>> +	'trace number increases')
> >>>> 
> >>>> Typo: I doubt we use tabs in Lua sources, but I might be wrong...
> >>> 
> >>> I suggest to use quarter tabs indent style as we use for C code and Mike
> >>> use in src/jit/*.lua (see bcsave.lua for example).
> >>> 
> >>> There was no precedent before, IINM :).
> >> 
> >> See test/tarantool-tests/lj-695-ffi-vararg-call.test.lua[1].
> > 
> > Fixed, repushed.
> > 
> >> 
> >>> 
> >>>> 
> >>>>> +
> >>>>> +ntrace_old = misc.getmetrics().jit_trace_num
> >>>>> +jit.on()
> >>>>> +irconst4()
> >>>>> +irconst4()
> >>>>> +jit.off()
> >>>>> +test:ok(ntrace_old == misc.getmetrics().jit_trace_num,
> >>>>> +	'trace number is the same')
> >>>> 
> >>>> Ditto.
> >>>> 
> >>>>> +
> >>>>> +os.exit(test:check() and 0 or 1)
> >>>>> -- 
> >>>>> 2.34.1
> >>>>> 
> >>>> 
> >>>> -- 
> >>>> Best regards,
> >>>> IM
> >>> 
> >>> -- 
> >>> Best regards,
> >>> Sergey Kaplun
> >> 
> >> [1]: https://github.com/tarantool/luajit/blob/tarantool/test/tarantool-tests/lj-695-ffi-vararg-call.test.lua
> >> 
> >> -- 
> >> Best regards,
> >> IM
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 

-- 
Best regards,
Sergey Kaplun