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

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

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


Hi, Sergey!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>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
> 

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