From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 86AC36DDFBA; Fri, 10 Nov 2023 15:49:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 86AC36DDFBA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1699620593; bh=6qyjGwP0tEMQieyrIko78oBiFLDeWvlJrvCFBvwWiIQ=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=QalmywaEUoYXFJRr+P8qtVuP12OzNCxArZtRMGQYkCDLveoy0ePDLZtuV2rKPIhP1 14Yh4grVtETnE3ln1hBHoVI4o17JfSnx1GDBXlDOKbAblGCCX58+fkiVNVioVzu+wt kr9JTKs6PmQq+XP9KHoLD0hoT3Bpr1dnzSZuec9U= Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [95.163.41.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 46B486DDCD5 for ; Fri, 10 Nov 2023 15:49:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 46B486DDCD5 Received: by smtp33.i.mail.ru with esmtpa (envelope-from ) id 1r1QxL-00GcMb-22; Fri, 10 Nov 2023 15:49:51 +0300 Date: Fri, 10 Nov 2023 15:49:51 +0300 To: Sergey Kaplun Message-ID: <5yynouo6jw5w27gtvrjqddg52in6wam6aqdcvptay4ivpq36bf@ng2qvitlre4f> References: <20231109122628.19853-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231109122628.19853-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9C2A6B03AB739174CD46FC98D30EEB53927C1B9ED3C31292500894C459B0CD1B974DC38A99336A87B20F9CF24C939550897D5EAB505E303BCA316B52795F7D3CA X-C1DE0DAB: 0D63561A33F958A57062A15EC6FC5CEB78699830A0868E66BEC899279ADCAEFDF87CCE6106E1FC07E67D4AC08A07B9B0DB8A315C1FF4794DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527FFEA18885791340B9C9E3817EFDCB5FB186DD3639F23E6F0D9BB4F712850405AD21E6A7F4CF831C5037FD76D11AF80A0D4C8EFD9D4C5743EBF91F3064BE03BFB1EA455F16B58544A2557BDE0DD54B3590965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj1d5ULkquG7xBajd4e+sGnQ== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE407E2C1CDD8205357AA8B41B386828DE924116B017870A99197D51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH luajit] Fix HREFK forwarding vs. table.clear(). X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the patch! Please consider my comments below. > 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 > +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. > +test_aref_fwd_tnew(2) > +-- Now run the trace with the clearing table, from which we take > +-- AREF. Same rephrasing required. Here and below. > +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. > + > +local function test_not_forwarded_hrefk_val_from_newref(tab_number) > + local field_value_after_clear > + for _ = 1, NITERATIONS do > + -- Create a table on trace to make the optimization work. > + local tab = {} > + -- NEWREF to be forwarded. > + 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. > + field_value_after_clear = tab.hrefk > + end > + 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 > + -- 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)/ > + 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. > + > +-- 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 >