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

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

Hi, Sergey!
Thanks for the fixes!
LGTM

On Tue, Dec 12, 2023 at 01:10:00PM +0300, Sergey Kaplun wrote:
> 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