Re: [Tarantool-patches] [PATCH luajit] Emit sunk IR_NEWREF only once per key on snapshot replay.

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

Hi, Maxim!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 12.12.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> 
> On Mon, Dec 11, 2023 at 06:35:20PM +0300, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Sergey Kaplun and Peter Cawley.
> >
> > (cherry-picked from commit 1761fd2ef79ffe1778011c7e9cb03ed361b48c5e)

<snipped>

> > +-- Uncompiled function to end up side trace here.
> > +local function trace_base_wp(num)
> > +  return trace_base(num)
> > +end
> > +jit.off(trace_base_wp)
> > +
> > +-- Same function as above, but with two IRs NEWREF emitted.
> Please mention that this test cases checks situation when last NEWREF
> is not the same.

Fixed, see the iterative patch below.

> > +local function trace_2newref(num)
> > +  local tab = {}
> > +  tab.key = false
> > +  -- This + op can't be folded since `num` can be -0.
> > +  tab.key = num + 0
> > +  tab.key2 = false
> > +  -- This check can't be folded since `num` can be NaN.
> > +  tab.key2 = num == num
> > +  -- luacheck: ignore
> > +  if take_side then end
> > +  return tab.key, tab.key2
> > +end
> Nit: `key` and `key2` naming seems a bit inconsistent.

Fixed, thanks!

===================================================================
diff --git a/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua b/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua
index a89beab6..77efd0f4 100644
--- a/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua
+++ b/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua
@@ -37,17 +37,18 @@ end
 jit.off(trace_base_wp)
 
 -- Same function as above, but with two IRs NEWREF emitted.
+-- The last NEWREF references another key.
 local function trace_2newref(num)
   local tab = {}
-  tab.key = false
+  tab.key1 = false
   -- This + op can't be folded since `num` can be -0.
-  tab.key = num + 0
+  tab.key1 = num + 0
   tab.key2 = false
   -- This check can't be folded since `num` can be NaN.
   tab.key2 = num == num
   -- luacheck: ignore
   if take_side then end
-  return tab.key, tab.key2
+  return tab.key1, tab.key2
 end
 
 -- Uncompiled function to end up side trace here.
===================================================================

> > +
> > +-- Uncompiled function to end up side trace here.
> > +local function trace_2newref_wp(num)
> > +  return trace_2newref(num)
> > +end
> > +jit.off(trace_2newref_wp)
> > +
> > +jit.opt.start('hotloop=1', 'hotexit=1', 'tryside=1')
> > +
> > +-- Compile parent traces.
> > +trace_base_wp(0)
> > +trace_base_wp(0)
> > +trace_2newref_wp(0)
> > +trace_2newref_wp(0)
> > +
> > +-- Compile side traces.
> > +take_side = true
> > +trace_base_wp(0)
> > +trace_base_wp(0)
> > +trace_2newref_wp(0)
> > +trace_2newref_wp(0)
> > +
> > +test:is(trace_base(0), true, 'sunk value restored correctly')
> > +
> > +local arg = 0
> > +local r1, r2 = trace_2newref(arg)
> > +test:is(r1, arg, 'sunk value restored correctly with 2 keys, first key')
> > +test:is(r2, true, 'sunk value restored correctly with 2 keys, second key')
> These assertions pass before the patch. Is this expected behavior? If
> so, please drop a comment.

Added:

===================================================================
diff --git a/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua b/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua
index a89beab6..77efd0f4 100644
--- a/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua
+++ b/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua
@@ -75,6 +76,8 @@ test:is(trace_base(0), true, 'sunk value restored correctly')
 
 local arg = 0
 local r1, r2 = trace_2newref(arg)
+-- These tests didn't fail before the patch.
+-- They check the patch's correctness.
 test:is(r1, arg, 'sunk value restored correctly with 2 keys, first key')
 test:is(r2, true, 'sunk value restored correctly with 2 keys, second key')
===================================================================

> > +
> > +test:done(true)
> > --
> > 2.43.0
> >

-- 
Best regards,
Sergey Kaplun