Re: [Tarantool-patches] [PATCH luajit] Fix HREFK forwarding vs. table.clear().

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

Hi, Maxim!
Thanks for the review!
Fixed your comments inline and force-pushed the branch.
See the iterative patch below.

On 10.11.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> 
> <snipped>
> 
> > diff --git a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
> > new file mode 100644
> > index 00000000..e662e0cc
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
> 
> <snipped>
> 
> > +jit.opt.start('hotloop=1')
> > +
> > +-- First compile the trace with a clearing not-interesting table.
> It is better to rephrase it the following way:
> | First, compile the trace that clears the not-interesting table.
> Otherwise, `clearing` seems to be an adjective referring to the table.
> Here and below.

Fixed, thanks!

> > +test_aref_fwd_tnew(2)
> > +-- Now run the trace with the clearing table, from which we take
> > +-- AREF.
> Same rephrasing required. Here and below.

Changed to "run the trace and clear the table ...".

> > +test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')
> > +
> > +-- XXX: Reset hotcounters to avoid collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +-- First compile the trace with a clearing not-interesting table.
> > +test_aref_fwd_tdup(2)
> > +-- Now run the trace with the clearing table, from which we take
> > +-- AREF.
> > +test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')
> > +
> > +-- XXX: Reset hotcounters to avoid collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +-- First compile the trace with a clearing not-interesting table.
> > +test_href_fwd_tnew(2)
> > +-- Now run the trace with the clearing table, from which we take
> > +-- HREF.
> > +test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')
> > +
> > +-- XXX: Reset hotcounters to avoid collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +-- First compile the trace with a clearing not-interesting table.
> > +test_href_fwd_tdup(2)
> > +-- Now run the trace with the clearing table, from which we take
> > +-- HREF.
> > +test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')
> 
> Maybe we can just iterate over those cases in a loop?
> If it breaks the semantics, then feel free to ignore.

Yes, the loop itself can have side effects, so I prefere to ignore it.

> 

<snipped>

> > +local function test_not_dropped_guard_on_hrefk(tab_number)
> > +  local tab, field_value_after_clear
> > +  for _ = 1, NITERATIONS do
> > +    -- Create a table on trace to make the optimization work.
> > +    tab = {hrefk = MAGIC}
> > +    -- Use an additional table to alias the created table with the
> > +    -- `hrefk` key.
> > +    local tab_array = {tab, {hrefk = 0}}
> > +    table_clear(tab_array[tab_number])
> > +    -- It should be `nil`, since it is cleared.
> > +    -- If the guard is dropped for HREFK, the value from the TDUP
> > +    -- table is taken instead, without the type check. This leads
> > +    -- to incorrect (swapped) returned values.
> Typo: s/incorrect (swapped) returned/incorrectly returned (swapped)/

Fixed.

> > +    field_value_after_clear = tab.hrefk
> > +    tab.hrefk = MAGIC
> > +  end
> > +  return field_value_after_clear, tab.hrefk
> > +end
> 
> Let's move those test case definitions to be right after the ones above,
> that would be more consistent.

Done.

> > +
> > +-- XXX: Reset hotcounters to avoid collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +-- First compile the trace with clearing not interesting table.
> > +test_not_dropped_guard_on_hrefk(2)
> > +-- Now run the trace with the clearing table, from which we take
> > +-- HREFK.
> > +local field_value_after_clear, tab_hrefk = test_not_dropped_guard_on_hrefk(1)
> > +
> > +test:is(field_value_after_clear, nil, 'correct field value after table.clear')
> > +test:is(tab_hrefk, MAGIC, 'correct value set in the table that was cleared')
> > +
> > +test:done(true)
> > -- 
> > 2.42.0
> > 

===================================================================
diff --git a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
index e662e0cc..e1fbc2ed 100644
--- a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
+++ b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
@@ -83,41 +83,6 @@ local function test_href_fwd_tdup(tab_number)
   return field_value_after_clear
 end
 
-jit.opt.start('hotloop=1')
-
--- First compile the trace with a clearing not-interesting table.
-test_aref_fwd_tnew(2)
--- Now run the trace with the clearing table, from which we take
--- AREF.
-test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')
-
--- XXX: Reset hotcounters to avoid collisions.
-jit.opt.start('hotloop=1')
-
--- First compile the trace with a clearing not-interesting table.
-test_aref_fwd_tdup(2)
--- Now run the trace with the clearing table, from which we take
--- AREF.
-test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')
-
--- XXX: Reset hotcounters to avoid collisions.
-jit.opt.start('hotloop=1')
-
--- First compile the trace with a clearing not-interesting table.
-test_href_fwd_tnew(2)
--- Now run the trace with the clearing table, from which we take
--- HREF.
-test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')
-
--- XXX: Reset hotcounters to avoid collisions.
-jit.opt.start('hotloop=1')
-
--- First compile the trace with a clearing not-interesting table.
-test_href_fwd_tdup(2)
--- Now run the trace with the clearing table, from which we take
--- HREF.
-test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')
-
 local function test_not_forwarded_hrefk_val_from_newref(tab_number)
   local field_value_after_clear
   for _ = 1, NITERATIONS do
@@ -135,18 +100,6 @@ local function test_not_forwarded_hrefk_val_from_newref(tab_number)
   return field_value_after_clear
 end
 
--- XXX: Reset hotcounters to avoid collisions.
-jit.opt.start('hotloop=1')
-
--- First compile the trace with a clearing not-interesting table.
-test_not_forwarded_hrefk_val_from_newref(2)
--- Now run the trace with the clearing table, from which we take
--- HREFK.
-local value_from_cleared_tab = test_not_forwarded_hrefk_val_from_newref(1)
-
-test:is(value_from_cleared_tab, nil,
-        'not forward the field value across table.clear')
-
 local function test_not_dropped_guard_on_hrefk(tab_number)
   local tab, field_value_after_clear
   for _ = 1, NITERATIONS do
@@ -159,19 +112,62 @@ local function test_not_dropped_guard_on_hrefk(tab_number)
     -- It should be `nil`, since it is cleared.
     -- If the guard is dropped for HREFK, the value from the TDUP
     -- table is taken instead, without the type check. This leads
-    -- to incorrect (swapped) returned values.
+    -- to incorrectly returned (swapped) values.
     field_value_after_clear = tab.hrefk
     tab.hrefk = MAGIC
   end
   return field_value_after_clear, tab.hrefk
 end
 
+jit.opt.start('hotloop=1')
+
+-- First, compile the trace that clears the not-interesting table.
+test_aref_fwd_tnew(2)
+-- Now run the trace and clear the table, from which we take AREF.
+test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')
+
+-- XXX: Reset hotcounters to avoid collisions.
+jit.opt.start('hotloop=1')
+
+-- First, compile the trace that clears the not-interesting table.
+test_aref_fwd_tdup(2)
+-- Now run the trace and clear the table, from which we take AREF.
+test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')
+
+-- XXX: Reset hotcounters to avoid collisions.
+jit.opt.start('hotloop=1')
+
+-- First, compile the trace that clears the not-interesting table.
+test_href_fwd_tnew(2)
+-- Now run the trace and clear the table, from which we take HREF.
+test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')
+
+-- XXX: Reset hotcounters to avoid collisions.
+jit.opt.start('hotloop=1')
+
+-- First, compile the trace that clears the not-interesting table.
+test_href_fwd_tdup(2)
+-- Now run the trace and clear the table, from which we take HREF.
+test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')
+
+-- XXX: Reset hotcounters to avoid collisions.
+jit.opt.start('hotloop=1')
+
+-- First, compile the trace that clears the not-interesting table.
+test_not_forwarded_hrefk_val_from_newref(2)
+-- Now run the trace and clear the table, from which we take
+-- HREFK.
+local value_from_cleared_tab = test_not_forwarded_hrefk_val_from_newref(1)
+
+test:is(value_from_cleared_tab, nil,
+        'not forward the field value across table.clear')
+
 -- XXX: Reset hotcounters to avoid collisions.
 jit.opt.start('hotloop=1')
 
--- First compile the trace with clearing not interesting table.
+-- First, compile the trace that clears the not-interesting table.
 test_not_dropped_guard_on_hrefk(2)
--- Now run the trace with the clearing table, from which we take
+-- Now run the trace and clear the table, from which we take
 -- HREFK.
 local field_value_after_clear, tab_hrefk = test_not_dropped_guard_on_hrefk(1)
 
===================================================================

-- 
Best regards,
Sergey Kaplun