[Tarantool-patches] [PATCH luajit 0/2] Fix tests for LUAJIT_ENABLE_CHECKHOOK

[Tarantool-patches] [PATCH luajit 0/2] Fix tests for LUAJIT_ENABLE_CHECKHOOK

[Igor Munkin via Tarantool-patches]

After enabling LUAJIT_ENABLE_CHECKHOOK options two tests started to fail
due to very tight JIT engine setup. This patchset is aimed to relax JIT
engine setup and make the tests agnostic to the introduced option.

Issue: https://github.com/tarantool/tarantool/issues/7762
Branch: https://github.com/tarantool/luajit/tree/imun/gh-7762-fix-tests-for-checkhook
PR: https://github.com/tarantool/tarantool/pull/7787
CI: https://github.com/tarantool/luajit/commit/ba9d9c8

Igor Munkin (2):
  test: relax JIT setup in lj-430-maxirconst test
  test: relax JIT setup in misc.getmetrics test

 .../lj-430-maxirconst.test.lua                | 23 +++++++++++--------
 .../misclib-getmetrics-capi.test.lua          |  7 ++++--
 .../misclib-getmetrics-lapi.test.lua          |  7 ++++--
 test/tarantool-tests/utils.lua                |  9 ++++++++
 4 files changed, 33 insertions(+), 13 deletions(-)

-- 
2.34.0

[Tarantool-patches] [PATCH luajit 1/2] test: relax JIT setup in lj-430-maxirconst test

[Igor Munkin via Tarantool-patches]

JIT engine setup in tarantool-tests/lj-430-maxirconst.test.lua is too
tight for the build with LUAJIT_ENABLE_CHECKHOOK option enabled.
Originally test limits <maxirconst> value with 3 for the default IRRef
constants set: nil, true, false. However, with LUAJIT_ENABLE_CHECKHOOK
enabled there are three more auxiliary IRRef constants for the internal
purposes. Hence, the test is reimplemented the following way:
* The first trace for the empty function is recorded
* The number of IRRef constants is obtained via <jit.util.traceinfo>
* <maxirconst> parameter is set to this value
* The second trace for the function with a single local variable,
  initialized with a constant, fails to record

As a result there is no need to explicitly respect any configuration.

Relates to tarantool/tarantool#7762

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 .../lj-430-maxirconst.test.lua                | 23 +++++++++++--------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/test/tarantool-tests/lj-430-maxirconst.test.lua b/test/tarantool-tests/lj-430-maxirconst.test.lua
index cd3587a8..eaa0cd1b 100644
--- a/test/tarantool-tests/lj-430-maxirconst.test.lua
+++ b/test/tarantool-tests/lj-430-maxirconst.test.lua
@@ -1,8 +1,3 @@
--- XXX: avoid any other traces compilation due to hotcount
--- collisions for predictable results.
-jit.off()
-jit.flush()
-
 -- Disabled on *BSD due to #4819.
 require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
 
@@ -12,10 +7,6 @@ local traceinfo = require('jit.util').traceinfo
 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
@@ -25,6 +16,12 @@ local function irconst4()
   local _ = 42
 end
 
+-- XXX: Avoid any other traces compilation due to hotcount
+-- collisions for predictable results.
+jit.off()
+jit.flush()
+jit.opt.start('hotloop=1')
+
 assert(not traceinfo(1), 'no traces compiled after flush')
 jit.on()
 irconst3()
@@ -32,6 +29,14 @@ irconst3()
 jit.off()
 test:ok(traceinfo(1), 'new trace created')
 
+-- XXX: Trace 1 always has at least 3 IR constants: for nil, false
+-- and true. In case when LUAJIT_ENABLE_CHECKHOOK is set to ON,
+-- three more constants are emitted to the trace.
+-- Tight up <maxirconst> and try to record the next trace with one
+-- more constant to be emitted.
+jit.opt.start(('maxirconst=%d'):format(traceinfo(1).nk))
+
+assert(not traceinfo(2), 'only one trace is created to this moment')
 jit.on()
 irconst4()
 irconst4()
-- 
2.34.0

Re: [Tarantool-patches] [PATCH luajit 1/2] test: relax JIT setup in lj-430-maxirconst test

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!

Thanks for the patch!

LGTM, just several nits regarding the commit message.

On 01.12.22, Igor Munkin wrote:
> JIT engine setup in tarantool-tests/lj-430-maxirconst.test.lua is too
> tight for the build with LUAJIT_ENABLE_CHECKHOOK option enabled.
> Originally test limits <maxirconst> value with 3 for the default IRRef
> constants set: nil, true, false. However, with LUAJIT_ENABLE_CHECKHOOK
> enabled there are three more auxiliary IRRef constants for the internal

Typo: s/enabled/enabled,/

> purposes. Hence, the test is reimplemented the following way:
> * The first trace for the empty function is recorded
> * The number of IRRef constants is obtained via <jit.util.traceinfo>
> * <maxirconst> parameter is set to this value
> * The second trace for the function with a single local variable,
>   initialized with a constant, fails to record
> 
> As a result there is no need to explicitly respect any configuration.

Typo: s/As a result/As a result,/

> 
> Relates to tarantool/tarantool#7762
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  .../lj-430-maxirconst.test.lua                | 23 +++++++++++--------
>  1 file changed, 14 insertions(+), 9 deletions(-)

<snipped>

> 2.34.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] test: relax JIT setup in lj-430-maxirconst test

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 05.12.22, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> LGTM, just several nits regarding the commit message.

Fixed both of them.

> 

<snipped>

> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 1/2] test: relax JIT setup in lj-430-maxirconst test

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Igor!
Thanks for the patch!
LGTM, except for the two nits below.
  
>Четверг, 1 декабря 2022, 23:42 +03:00 от Igor Munkin <imun@tarantool.org>:
> 
>JIT engine setup in tarantool-tests/lj-430-maxirconst.test.lua is too
>tight for the build with LUAJIT_ENABLE_CHECKHOOK option enabled.
>Originally test limits <maxirconst> value with 3 for the default IRRef
>constants set: nil, true, false. However, with LUAJIT_ENABLE_CHECKHOOK
>enabled there are three more auxiliary IRRef constants for the internal
>purposes. Hence, the test is reimplemented the following way:
>* The first trace for the empty function is recorded
>* The number of IRRef constants is obtained via <jit.util.traceinfo>
>* <maxirconst> parameter is set to this value
>* The second trace for the function with a single local variable,
>  initialized with a constant, fails to record
>
>As a result there is no need to explicitly respect any configuration.
>
>Relates to tarantool/tarantool#7762
>
>Signed-off-by: Igor Munkin < imun@tarantool.org >
>---
> .../lj-430-maxirconst.test.lua | 23 +++++++++++--------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
>diff --git a/test/tarantool-tests/lj-430-maxirconst.test.lua b/test/tarantool-tests/lj-430-maxirconst.test.lua
>index cd3587a8..eaa0cd1b 100644
>--- a/test/tarantool-tests/lj-430-maxirconst.test.lua
>+++ b/test/tarantool-tests/lj-430-maxirconst.test.lua
>@@ -1,8 +1,3 @@
>--- XXX: avoid any other traces compilation due to hotcount
>--- collisions for predictable results.
>-jit.off()
>-jit.flush()
>-
> -- Disabled on *BSD due to #4819.
> require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> 
>@@ -12,10 +7,6 @@ local traceinfo = require('jit.util').traceinfo
> 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
>@@ -25,6 +16,12 @@ local function irconst4()
>   local _ = 42
> end
> 
>+-- XXX: Avoid any other traces compilation due to hotcount
>+-- collisions for predictable results.
>+jit.off()
>+jit.flush()
>+jit.opt.start('hotloop=1')
>+
> assert(not traceinfo(1), 'no traces compiled after flush')
> jit.on()
> irconst3()
>@@ -32,6 +29,14 @@ irconst3()
> jit.off()
> test:ok(traceinfo(1), 'new trace created')
> 
>+-- XXX: Trace 1 always has at least 3 IR constants: for nil, false
>+-- and true. In case when LUAJIT_ENABLE_CHECKHOOK is set to ON,
Typo: s/In case when/If/
>+-- three more constants are emitted to the trace.
>+-- Tight up <maxirconst> and try to record the next trace with one
Typo: s/Tight up/Tighten up/
>+-- more constant to be emitted.
>+jit.opt.start(('maxirconst=%d'):format(traceinfo(1).nk))
>+
>+assert(not traceinfo(2), 'only one trace is created to this moment')
> jit.on()
> irconst4()
> irconst4()
>--
>2.34.0
--
Best regards,
Maxim Kokryashkin

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

Re: [Tarantool-patches] [PATCH luajit 1/2] test: relax JIT setup in lj-430-maxirconst test

[Igor Munkin via Tarantool-patches]

Max,

Thanks for your review!

On 05.12.22, Maxim Kokryashkin wrote:
> 
> Hi, Igor!
> Thanks for the patch!
> LGTM, except for the two nits below.

Added your tag:
| Reviewed-by: Maxim Kokryashkin <m.kokryashkin@tarantool.org>

Fixed all your nits.

>   

<snipped>

> -- 
> Best regards,
> Maxim Kokryashkin

-- 
Best regards,
IM

[Tarantool-patches] [PATCH luajit 2/2] test: relax JIT setup in misc.getmetrics test

[Igor Munkin via Tarantool-patches]

To test whether <jit_snap_restore> metric is counted right for side
exits <minstitch> JIT engine parameter is tighted to prevent side trace
compilation (considering <math.fmod> being used for the corresponding
branch of execution, the side trace will try to compile with stitching).
However, if test is run on the platform configured with the option
LUAJIT_ENABLE_CHECKHOOK enabled, the resulting trace exceeds the
hardcoded <minstitch> limit and the metric is miscounted due to this new
trace. To resolve this issue <minstitch> limit is set to the maximum
amount of IRRef in the trace, preventing trace compilation for any
LuaJIT configuration.

Relates to tarantool/tarantool#7762

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 test/tarantool-tests/misclib-getmetrics-capi.test.lua | 7 +++++--
 test/tarantool-tests/misclib-getmetrics-lapi.test.lua | 7 +++++--
 test/tarantool-tests/utils.lua                        | 9 +++++++++
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
index 0cbc6cae..42a9adf9 100644
--- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
+++ b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
@@ -1,5 +1,7 @@
+local utils = require('utils')
+
 -- Disabled on *BSD due to #4819.
-require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
+utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
 
 local path = arg[0]:gsub('%.test%.lua', '')
 local suffix = package.cpath:match('?.(%a+);')
@@ -94,7 +96,8 @@ end))
 
 -- Compiled loop with a side exit which does not get compiled.
 test:ok(testgetmetrics.snap_restores(function()
-    jit.opt.start(0, "hotloop=1", "hotexit=2", "minstitch=15")
+    jit.opt.start(0, "hotloop=1", "hotexit=2",
+                  ("minstitch=%d"):format(utils.const.maxnins))
 
     local function foo(i)
         -- math.fmod is not yet compiled!
diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
index 222e7d01..19dfd199 100644
--- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
+++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
@@ -2,8 +2,10 @@
 -- Major portions taken verbatim or adapted from the LuaVela testing suite.
 -- Copyright (C) 2015-2019 IPONWEB Ltd.
 
+local utils = require('utils')
+
 -- Disabled on *BSD due to #4819.
-require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
+utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
 
 local tap = require('tap')
 
@@ -279,7 +281,8 @@ test:test("snap-restores-loop-side-exit-non-compiled", function(subtest)
     -- Compiled loop with a side exit which does not get compiled.
     subtest:plan(1)
 
-    jit.opt.start(0, "hotloop=1", "hotexit=2", "minstitch=15")
+    jit.opt.start(0, "hotloop=1", "hotexit=2",
+                  ("minstitch=%d"):format(utils.const.maxnins))
 
     local function foo(i)
         -- math.fmod is not yet compiled!
diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
index 87f7ff15..eb11d40d 100644
--- a/test/tarantool-tests/utils.lua
+++ b/test/tarantool-tests/utils.lua
@@ -135,4 +135,13 @@ function M.profilename(name)
   return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern))
 end
 
+M.const = {
+  -- XXX: Max nins is limited by max IRRef, that equals to
+  -- REF_DROP - REF_BIAS. Unfortunately, these constants are not
+  -- provided to Lua space, so we ought to make some math:
+  -- * REF_DROP = 0xffff
+  -- * REF_BIAS = 0x8000
+  maxnins = 0xffff - 0x8000,
+}
+
 return M
-- 
2.34.0

Re: [Tarantool-patches] [PATCH luajit 2/2] test: relax JIT setup in misc.getmetrics test

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!

Thanks for the patch!

LGTM, except a single nit regarding the commit message.

On 01.12.22, Igor Munkin wrote:
> To test whether <jit_snap_restore> metric is counted right for side
> exits <minstitch> JIT engine parameter is tighted to prevent side trace
> compilation (considering <math.fmod> being used for the corresponding
> branch of execution, the side trace will try to compile with stitching).
> However, if test is run on the platform configured with the option

Typo: s/test/the test/

> LUAJIT_ENABLE_CHECKHOOK enabled, the resulting trace exceeds the
> hardcoded <minstitch> limit and the metric is miscounted due to this new
> trace. To resolve this issue <minstitch> limit is set to the maximum
> amount of IRRef in the trace, preventing trace compilation for any
> LuaJIT configuration.
> 
> Relates to tarantool/tarantool#7762
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  test/tarantool-tests/misclib-getmetrics-capi.test.lua | 7 +++++--
>  test/tarantool-tests/misclib-getmetrics-lapi.test.lua | 7 +++++--
>  test/tarantool-tests/utils.lua                        | 9 +++++++++
>  3 files changed, 19 insertions(+), 4 deletions(-)
> 

<snipped>

> -- 
> 2.34.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2] test: relax JIT setup in misc.getmetrics test

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 05.12.22, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> 
> LGTM, except a single nit regarding the commit message.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

The nit is fixed.

> 

<snipped>

> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/2] test: relax JIT setup in misc.getmetrics test

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Igor!
Thanks for the patch!
LGTM, except for the single nit Sergey has already mentioned.
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>To test whether <jit_snap_restore> metric is counted right for side
>>exits <minstitch> JIT engine parameter is tighted to prevent side trace
>>compilation (considering <math.fmod> being used for the corresponding
>>branch of execution, the side trace will try to compile with stitching).
>>However, if test is run on the platform configured with the option
>>LUAJIT_ENABLE_CHECKHOOK enabled, the resulting trace exceeds the
>>hardcoded <minstitch> limit and the metric is miscounted due to this new
>>trace. To resolve this issue <minstitch> limit is set to the maximum
>>amount of IRRef in the trace, preventing trace compilation for any
>>LuaJIT configuration.
>>
>>Relates to tarantool/tarantool#7762
>>
>>Signed-off-by: Igor Munkin < imun@tarantool.org >
>>---
>> test/tarantool-tests/misclib-getmetrics-capi.test.lua | 7 +++++--
>> test/tarantool-tests/misclib-getmetrics-lapi.test.lua | 7 +++++--
>> test/tarantool-tests/utils.lua | 9 +++++++++
>> 3 files changed, 19 insertions(+), 4 deletions(-)
>>
>>diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
>>index 0cbc6cae..42a9adf9 100644
>>--- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
>>+++ b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
>>@@ -1,5 +1,7 @@
>>+local utils = require('utils')
>>+
>> -- Disabled on *BSD due to #4819.
>>-require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>>+utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>> 
>> local path = arg[0]:gsub('%.test%.lua', '')
>> local suffix = package.cpath:match('?.(%a+);')
>>@@ -94,7 +96,8 @@ end))
>> 
>> -- Compiled loop with a side exit which does not get compiled.
>> test:ok(testgetmetrics.snap_restores(function()
>>- jit.opt.start(0, "hotloop=1", "hotexit=2", "minstitch=15")
>>+ jit.opt.start(0, "hotloop=1", "hotexit=2",
>>+ ("minstitch=%d"):format(utils.const.maxnins))
>> 
>>     local function foo(i)
>>         -- math.fmod is not yet compiled!
>>diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
>>index 222e7d01..19dfd199 100644
>>--- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
>>+++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
>>@@ -2,8 +2,10 @@
>> -- Major portions taken verbatim or adapted from the LuaVela testing suite.
>> -- Copyright (C) 2015-2019 IPONWEB Ltd.
>> 
>>+local utils = require('utils')
>>+
>> -- Disabled on *BSD due to #4819.
>>-require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>>+utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>> 
>> local tap = require('tap')
>> 
>>@@ -279,7 +281,8 @@ test:test("snap-restores-loop-side-exit-non-compiled", function(subtest)
>>     -- Compiled loop with a side exit which does not get compiled.
>>     subtest:plan(1)
>> 
>>- jit.opt.start(0, "hotloop=1", "hotexit=2", "minstitch=15")
>>+ jit.opt.start(0, "hotloop=1", "hotexit=2",
>>+ ("minstitch=%d"):format(utils.const.maxnins))
>> 
>>     local function foo(i)
>>         -- math.fmod is not yet compiled!
>>diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
>>index 87f7ff15..eb11d40d 100644
>>--- a/test/tarantool-tests/utils.lua
>>+++ b/test/tarantool-tests/utils.lua
>>@@ -135,4 +135,13 @@ function M.profilename(name)
>>   return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern))
>> end
>> 
>>+M.const = {
>>+ -- XXX: Max nins is limited by max IRRef, that equals to
>>+ -- REF_DROP - REF_BIAS. Unfortunately, these constants are not
>>+ -- provided to Lua space, so we ought to make some math:
>>+ -- * REF_DROP = 0xffff
>>+ -- * REF_BIAS = 0x8000
>>+ maxnins = 0xffff - 0x8000,
>>+}
>>+
>> return M
>>--
>>2.34.0
> 

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

Re: [Tarantool-patches] [PATCH luajit 2/2] test: relax JIT setup in misc.getmetrics test

[Igor Munkin via Tarantool-patches]

Max,

Thanks for your review!

On 05.12.22, Maxim Kokryashkin wrote:
> 
> Hi, Igor!
> Thanks for the patch!
> LGTM, except for the single nit Sergey has already mentioned.

Added your tag:
| Reviewed-by: Maxim Kokryashkin <m.kokryashkin@tarantool.org>

> --
> Best regards,
> Maxim Kokryashkin

<snipped>

> > 

-- 
Best regards,
IM

[Tarantool-patches] [PATCH luajit 3/2] test: remove TAP side effects in getmetrics tests

[Igor Munkin via Tarantool-patches]

Sometimes TAP functions become hot spots for JIT compiler and the
corresponding traces spoils test assertions with their side effects.
To avoid such misbehaviour and fix fragile test, <misc.getmetrics> is
explicitly called in this particular case. There is no need to fix the
corresponding test for Lua C API interface, since there is no TAP
helpers used to check whether <jit_snap_restore> is counted right. There
is also no need to fix other subtests nearby, since their assertions are
no affected by TAP helpers side effects.

Relates to tarantool/tarantool#7762

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 test/tarantool-tests/misclib-getmetrics-lapi.test.lua | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
index 19dfd199..0c170d0c 100644
--- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
+++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
@@ -363,7 +363,9 @@ test:test("snap-restores-scalar", function(subtest)
     -- No exits triggering snap restore so far: snapshot
     -- restoration was inlined into the machine code.
     subtest:is(new_metrics.jit_snap_restore - old_metrics.jit_snap_restore, 0)
-    old_metrics = new_metrics
+    -- XXX: obtain actual metrics to avoid side effects that are
+    -- caused by Lua code and JIT engine fine tuning above.
+    old_metrics = misc.getmetrics()
 
     -- Simply 2 side exits from the trace:
     foo(20)
-- 
2.34.0

Re: [Tarantool-patches] [PATCH luajit 3/2] test: remove TAP side effects in getmetrics tests

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!

Thanks for the patch!

LGTM, except a few nits regarding the commit message.

On 02.12.22, Igor Munkin wrote:
> Sometimes TAP functions become hot spots for JIT compiler and the
> corresponding traces spoils test assertions with their side effects.

Typo: s/spoils/spoil/

> To avoid such misbehaviour and fix fragile test, <misc.getmetrics> is
> explicitly called in this particular case. There is no need to fix the
> corresponding test for Lua C API interface, since there is no TAP

Typo: s/Lua C API/the Lua C API/
Typo: s/is/are/

> helpers used to check whether <jit_snap_restore> is counted right. There
> is also no need to fix other subtests nearby, since their assertions are
> no affected by TAP helpers side effects.
> 
> Relates to tarantool/tarantool#7762
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---

<snipped>

> -- 
> 2.34.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 3/2] test: remove TAP side effects in getmetrics tests

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 05.12.22, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> 
> LGTM, except a few nits regarding the commit message.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

All nits are fixed.

> 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 3/2] test: remove TAP side effects in getmetrics tests

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Igor!
Thanks for the patch!
LGTM, except for the nits Sergey has already mentioned
and the two additional nits below.
 
> 
>>Sometimes TAP functions become hot spots for JIT compiler and the
>Typo: s/for JIT compiler/for the JIT compiler/
>>corresponding traces spoils test assertions with their side effects.
>>To avoid such misbehaviour and fix fragile test, <misc.getmetrics> is
>>explicitly called in this particular case. There is no need to fix the
>>corresponding test for Lua C API interface, since there is no TAP
>>helpers used to check whether <jit_snap_restore> is counted right. There
>>is also no need to fix other subtests nearby, since their assertions are
>>no affected by TAP helpers side effects.
>Typo: s/no/not/
>>
>>Relates to tarantool/tarantool#7762
>>
>>Signed-off-by: Igor Munkin < imun@tarantool.org >
>>---
>> test/tarantool-tests/misclib-getmetrics-lapi.test.lua | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>>diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
>>index 19dfd199..0c170d0c 100644
>>--- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
>>+++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
>>@@ -363,7 +363,9 @@ test:test("snap-restores-scalar", function(subtest)
>>     -- No exits triggering snap restore so far: snapshot
>>     -- restoration was inlined into the machine code.
>>     subtest:is(new_metrics.jit_snap_restore - old_metrics.jit_snap_restore, 0)
>>- old_metrics = new_metrics
>>+ -- XXX: obtain actual metrics to avoid side effects that are
>>+ -- caused by Lua code and JIT engine fine tuning above.
>>+ old_metrics = misc.getmetrics()
>> 
>>     -- Simply 2 side exits from the trace:
>>     foo(20)
>>--
>>2.34.0
>--
>Best regards,
>Maxim Kokryashkin

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

Re: [Tarantool-patches] [PATCH luajit 3/2] test: remove TAP side effects in getmetrics tests

[Igor Munkin via Tarantool-patches]

Max,

Thanks for your review!

On 05.12.22, Maxim Kokryashkin wrote:
> 
> Hi, Igor!
> Thanks for the patch!
> LGTM, except for the nits Sergey has already mentioned
> and the two additional nits below.

Added your tag:
| Reviewed-by: Maxim Kokryashkin <m.kokryashkin@tarantool.org>

All nits are fixed.

>  

<snipped>

> >--
> >Best regards,
> >Maxim Kokryashkin

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 0/2] Fix tests for LUAJIT_ENABLE_CHECKHOOK

[Igor Munkin via Tarantool-patches]

I've extended the series with additional patch, fixing flaky subtest in
test/tarantool-tests/misclib-getmetrics-lapi.test.lua. I failed to find
the root cause why it became flaky as a result of enabling the new
option LUAJIT_ENABLE_CHECKHOOK, but the test was implemented in a
fragile way, so its flakiness was a matter of time. Fixed now, updated
both LuaJIT branch and Tarantool PR.

On 01.12.22, Igor Munkin wrote:
> After enabling LUAJIT_ENABLE_CHECKHOOK options two tests started to fail
> due to very tight JIT engine setup. This patchset is aimed to relax JIT
> engine setup and make the tests agnostic to the introduced option.
> 
> Issue: https://github.com/tarantool/tarantool/issues/7762
> Branch: https://github.com/tarantool/luajit/tree/imun/gh-7762-fix-tests-for-checkhook
> PR: https://github.com/tarantool/tarantool/pull/7787
> CI: https://github.com/tarantool/luajit/commit/ba9d9c8
> 
> Igor Munkin (2):
>   test: relax JIT setup in lj-430-maxirconst test
>   test: relax JIT setup in misc.getmetrics test
> 
>  .../lj-430-maxirconst.test.lua                | 23 +++++++++++--------
>  .../misclib-getmetrics-capi.test.lua          |  7 ++++--
>  .../misclib-getmetrics-lapi.test.lua          |  7 ++++--
>  test/tarantool-tests/utils.lua                |  9 ++++++++
>  4 files changed, 33 insertions(+), 13 deletions(-)
> 
> -- 
> 2.34.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 0/2] Fix tests for LUAJIT_ENABLE_CHECKHOOK

[Igor Munkin via Tarantool-patches]

I've checked the patches into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

On 01.12.22, Igor Munkin wrote:
> After enabling LUAJIT_ENABLE_CHECKHOOK options two tests started to fail
> due to very tight JIT engine setup. This patchset is aimed to relax JIT
> engine setup and make the tests agnostic to the introduced option.
> 
> Issue: https://github.com/tarantool/tarantool/issues/7762
> Branch: https://github.com/tarantool/luajit/tree/imun/gh-7762-fix-tests-for-checkhook
> PR: https://github.com/tarantool/tarantool/pull/7787
> CI: https://github.com/tarantool/luajit/commit/ba9d9c8
> 
> Igor Munkin (2):
>   test: relax JIT setup in lj-430-maxirconst test
>   test: relax JIT setup in misc.getmetrics test
> 
>  .../lj-430-maxirconst.test.lua                | 23 +++++++++++--------
>  .../misclib-getmetrics-capi.test.lua          |  7 ++++--
>  .../misclib-getmetrics-lapi.test.lua          |  7 ++++--
>  test/tarantool-tests/utils.lua                |  9 ++++++++
>  4 files changed, 33 insertions(+), 13 deletions(-)
> 
> -- 
> 2.34.0
> 

-- 
Best regards,
IM