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

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

Igor,

Thanks for the review!

On 28.01.22, Igor Munkin wrote:
> 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/.

Ouch! Fixed.

> 
> > 
> > `maxirconst` should restrict the amount of IR constants for 1 trace.
> 
> Typo: It's better s/for 1/per/, but feel free to ignore.

Fixed.

The new commit message is the following:

===================================================================
Actually implement maxirconst trace limit.

Suggested by spacewander.

(cherry picked from 0a9ff94c4a1fcec2c310dcb092da694f23186e23)

`maxirconst` should restrict the amount of IR constants per trace.
Nevertheless, its value isn't checked anywhere.

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

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

Fixed.

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 1829b37d..df17637f 100644
--- a/test/tarantool-tests/lj-430-maxirconst.test.lua
+++ b/test/tarantool-tests/lj-430-maxirconst.test.lua
@@ -1,5 +1,5 @@
 -- XXX: avoid any other traces compilation due to hotcount
--- collisions for predictible results.
+-- collisions for predictable results.
 jit.off()
 jit.flush()
 
===================================================================

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

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

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 :).

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