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