<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the fixes!</div><div>LGTM</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16998613462134056469_BODY">Hi, Maxim!<br>Thanks for the review!<br>Fixed your comments inline and force-pushed the branch.<br>See the iterative patch below.<br><br>On 10.11.23, Maxim Kokryashkin wrote:<br>> Hi, Sergey!<br>> Thanks for the patch!<br>> Please consider my comments below.<br>><br>> <snipped><br>><br>> > diff --git a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua<br>> > new file mode 100644<br>> > index 00000000..e662e0cc<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua<br>><br>> <snipped><br>><br>> > +jit.opt.start('hotloop=1')<br>> > +<br>> > +-- First compile the trace with a clearing not-interesting table.<br>> It is better to rephrase it the following way:<br>> | First, compile the trace that clears the not-interesting table.<br>> Otherwise, `clearing` seems to be an adjective referring to the table.<br>> Here and below.<br><br>Fixed, thanks!<br><br>> > +test_aref_fwd_tnew(2)<br>> > +-- Now run the trace with the clearing table, from which we take<br>> > +-- AREF.<br>> Same rephrasing required. Here and below.<br><br>Changed to "run the trace and clear the table ...".<br><br>> > +test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')<br>> > +<br>> > +-- XXX: Reset hotcounters to avoid collisions.<br>> > +jit.opt.start('hotloop=1')<br>> > +<br>> > +-- First compile the trace with a clearing not-interesting table.<br>> > +test_aref_fwd_tdup(2)<br>> > +-- Now run the trace with the clearing table, from which we take<br>> > +-- AREF.<br>> > +test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')<br>> > +<br>> > +-- XXX: Reset hotcounters to avoid collisions.<br>> > +jit.opt.start('hotloop=1')<br>> > +<br>> > +-- First compile the trace with a clearing not-interesting table.<br>> > +test_href_fwd_tnew(2)<br>> > +-- Now run the trace with the clearing table, from which we take<br>> > +-- HREF.<br>> > +test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')<br>> > +<br>> > +-- XXX: Reset hotcounters to avoid collisions.<br>> > +jit.opt.start('hotloop=1')<br>> > +<br>> > +-- First compile the trace with a clearing not-interesting table.<br>> > +test_href_fwd_tdup(2)<br>> > +-- Now run the trace with the clearing table, from which we take<br>> > +-- HREF.<br>> > +test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')<br>><br>> Maybe we can just iterate over those cases in a loop?<br>> If it breaks the semantics, then feel free to ignore.<br><br>Yes, the loop itself can have side effects, so I prefere to ignore it.<br><br>><br><br><snipped><br><br>> > +local function test_not_dropped_guard_on_hrefk(tab_number)<br>> > + local tab, field_value_after_clear<br>> > + for _ = 1, NITERATIONS do<br>> > + -- Create a table on trace to make the optimization work.<br>> > + tab = {hrefk = MAGIC}<br>> > + -- Use an additional table to alias the created table with the<br>> > + -- `hrefk` key.<br>> > + local tab_array = {tab, {hrefk = 0}}<br>> > + table_clear(tab_array[tab_number])<br>> > + -- It should be `nil`, since it is cleared.<br>> > + -- If the guard is dropped for HREFK, the value from the TDUP<br>> > + -- table is taken instead, without the type check. This leads<br>> > + -- to incorrect (swapped) returned values.<br>> Typo: s/incorrect (swapped) returned/incorrectly returned (swapped)/<br><br>Fixed.<br><br>> > + field_value_after_clear = tab.hrefk<br>> > + tab.hrefk = MAGIC<br>> > + end<br>> > + return field_value_after_clear, tab.hrefk<br>> > +end<br>><br>> Let's move those test case definitions to be right after the ones above,<br>> that would be more consistent.<br><br>Done.<br><br>> > +<br>> > +-- XXX: Reset hotcounters to avoid collisions.<br>> > +jit.opt.start('hotloop=1')<br>> > +<br>> > +-- First compile the trace with clearing not interesting table.<br>> > +test_not_dropped_guard_on_hrefk(2)<br>> > +-- Now run the trace with the clearing table, from which we take<br>> > +-- HREFK.<br>> > +local field_value_after_clear, tab_hrefk = test_not_dropped_guard_on_hrefk(1)<br>> > +<br>> > +test:is(field_value_after_clear, nil, 'correct field value after table.clear')<br>> > +test:is(tab_hrefk, MAGIC, 'correct value set in the table that was cleared')<br>> > +<br>> > +test:done(true)<br>> > --<br>> > 2.42.0<br>> ><br><br>===================================================================<br>diff --git a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua<br>index e662e0cc..e1fbc2ed 100644<br>--- a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua<br>+++ b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua<br>@@ -83,41 +83,6 @@ local function test_href_fwd_tdup(tab_number)<br> return field_value_after_clear<br> end<br> <br>-jit.opt.start('hotloop=1')<br>-<br>--- First compile the trace with a clearing not-interesting table.<br>-test_aref_fwd_tnew(2)<br>--- Now run the trace with the clearing table, from which we take<br>--- AREF.<br>-test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')<br>-<br>--- XXX: Reset hotcounters to avoid collisions.<br>-jit.opt.start('hotloop=1')<br>-<br>--- First compile the trace with a clearing not-interesting table.<br>-test_aref_fwd_tdup(2)<br>--- Now run the trace with the clearing table, from which we take<br>--- AREF.<br>-test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')<br>-<br>--- XXX: Reset hotcounters to avoid collisions.<br>-jit.opt.start('hotloop=1')<br>-<br>--- First compile the trace with a clearing not-interesting table.<br>-test_href_fwd_tnew(2)<br>--- Now run the trace with the clearing table, from which we take<br>--- HREF.<br>-test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')<br>-<br>--- XXX: Reset hotcounters to avoid collisions.<br>-jit.opt.start('hotloop=1')<br>-<br>--- First compile the trace with a clearing not-interesting table.<br>-test_href_fwd_tdup(2)<br>--- Now run the trace with the clearing table, from which we take<br>--- HREF.<br>-test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')<br>-<br> local function test_not_forwarded_hrefk_val_from_newref(tab_number)<br> local field_value_after_clear<br> for _ = 1, NITERATIONS do<br>@@ -135,18 +100,6 @@ local function test_not_forwarded_hrefk_val_from_newref(tab_number)<br> return field_value_after_clear<br> end<br> <br>--- XXX: Reset hotcounters to avoid collisions.<br>-jit.opt.start('hotloop=1')<br>-<br>--- First compile the trace with a clearing not-interesting table.<br>-test_not_forwarded_hrefk_val_from_newref(2)<br>--- Now run the trace with the clearing table, from which we take<br>--- HREFK.<br>-local value_from_cleared_tab = test_not_forwarded_hrefk_val_from_newref(1)<br>-<br>-test:is(value_from_cleared_tab, nil,<br>- 'not forward the field value across table.clear')<br>-<br> local function test_not_dropped_guard_on_hrefk(tab_number)<br> local tab, field_value_after_clear<br> for _ = 1, NITERATIONS do<br>@@ -159,19 +112,62 @@ local function test_not_dropped_guard_on_hrefk(tab_number)<br> -- It should be `nil`, since it is cleared.<br> -- If the guard is dropped for HREFK, the value from the TDUP<br> -- table is taken instead, without the type check. This leads<br>- -- to incorrect (swapped) returned values.<br>+ -- to incorrectly returned (swapped) values.<br> field_value_after_clear = tab.hrefk<br> tab.hrefk = MAGIC<br> end<br> return field_value_after_clear, tab.hrefk<br> end<br> <br>+jit.opt.start('hotloop=1')<br>+<br>+-- First, compile the trace that clears the not-interesting table.<br>+test_aref_fwd_tnew(2)<br>+-- Now run the trace and clear the table, from which we take AREF.<br>+test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')<br>+<br>+-- XXX: Reset hotcounters to avoid collisions.<br>+jit.opt.start('hotloop=1')<br>+<br>+-- First, compile the trace that clears the not-interesting table.<br>+test_aref_fwd_tdup(2)<br>+-- Now run the trace and clear the table, from which we take AREF.<br>+test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')<br>+<br>+-- XXX: Reset hotcounters to avoid collisions.<br>+jit.opt.start('hotloop=1')<br>+<br>+-- First, compile the trace that clears the not-interesting table.<br>+test_href_fwd_tnew(2)<br>+-- Now run the trace and clear the table, from which we take HREF.<br>+test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')<br>+<br>+-- XXX: Reset hotcounters to avoid collisions.<br>+jit.opt.start('hotloop=1')<br>+<br>+-- First, compile the trace that clears the not-interesting table.<br>+test_href_fwd_tdup(2)<br>+-- Now run the trace and clear the table, from which we take HREF.<br>+test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')<br>+<br>+-- XXX: Reset hotcounters to avoid collisions.<br>+jit.opt.start('hotloop=1')<br>+<br>+-- First, compile the trace that clears the not-interesting table.<br>+test_not_forwarded_hrefk_val_from_newref(2)<br>+-- Now run the trace and clear the table, from which we take<br>+-- HREFK.<br>+local value_from_cleared_tab = test_not_forwarded_hrefk_val_from_newref(1)<br>+<br>+test:is(value_from_cleared_tab, nil,<br>+ 'not forward the field value across table.clear')<br>+<br> -- XXX: Reset hotcounters to avoid collisions.<br> jit.opt.start('hotloop=1')<br> <br>--- First compile the trace with clearing not interesting table.<br>+-- First, compile the trace that clears the not-interesting table.<br> test_not_dropped_guard_on_hrefk(2)<br>--- Now run the trace with the clearing table, from which we take<br>+-- Now run the trace and clear the table, from which we take<br> -- HREFK.<br> local field_value_after_clear, tab_hrefk = test_not_dropped_guard_on_hrefk(1)<br> <br>===================================================================<br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>