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

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

[-- Attachment #1: Type: text/plain, Size: 3182 bytes --]

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’

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


[-- Attachment #2: Type: text/html, Size: 12356 bytes --]