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

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

Sergey,

Thanks for the patch! LGTM, except a couple of nits.

On 29.12.21, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Suggested by spacewander.
> 
> (cherry picked from 9ff94c4a1fcec2c310dcb092da694f23186e23)

Typo: s/9ff94c4a1fcec2c310dcb092da694f23186e23/0a9ff94c4a1fcec2c310dcb092da694f23186e23/.

> 
> `maxirconst` should restrict the amount of IR constants for 1 trace.

Typo: It's better s/for 1/per/, but feel free to ignore.

> Nevertheless, its value isn't checked anywhere.

Lol, classic.

> 
> This patch adds the corresponding check after instruction recording.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#6548
> ---
> 
> 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.

Typo: s/predictible/predictable/.

> +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.

> +
> +-- Disabled on *BSD due to #4819.
> +require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +
> +local tap = require('tap')
> +
> +local test = tap.test('lj-430-maxirconst')
> +test:plan(2)
> +
> +-- XXX: trace always has at least 3 IR constants: for nil, false
> +-- and true.
> +jit.opt.start('hotloop=1', 'maxirconst=3')
> +
> +-- This function has only 3 IR constant.
> +local function irconst3()
> +end
> +
> +-- This function has 4 IR constants before optimizations.
> +local function irconst4()
> +  local _ = 42
> +end
> +
> +local ntrace_old = misc.getmetrics().jit_trace_num
> +jit.on()
> +irconst3()
> +irconst3()
> +jit.off()
> +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...

> +
> +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