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 3431572FFD4; Tue, 12 Dec 2023 14:45:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3431572FFD4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1702381544; bh=uWu/5D/Cgjk0noQX77iRULbO30+V7/3uQQwqREH+8OA=; 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=Zf3Nevdtw9JZmpGATVi384G/R46bBq9Bi8zUAZwXK/rW2qB6XgMJBjPYWw4qOwrRg Alzxh2qrgbQ53OCGwe5W99vQlXqh/J6A628n47Znj3yxImt9Wr4wjvtish5Z+VFn6m 9PsbzZC4v6UBp0rrwDRlnnNP0agRr9Eik21MDaJ8= Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [95.163.41.66]) (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 860C572FA48 for ; Tue, 12 Dec 2023 14:45:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 860C572FA48 Received: by smtp43.i.mail.ru with esmtpa (envelope-from ) id 1rD1Cn-0070NT-2n; Tue, 12 Dec 2023 14:45:42 +0300 Date: Tue, 12 Dec 2023 14:45:41 +0300 To: Sergey Kaplun Message-ID: References: <20231211153520.9322-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD97533543916A0F71AAFEA554151D011C2857CB10EE404042BCD62213F67905E7A73D8427066CEB5EEA1A8797A420DDB1780D02BBD8DFEF800E2602FFE9E909B31 X-C1DE0DAB: 0D63561A33F958A539D5F8ACA5E84EA04C832885FA43F1E5CC8C7CF9C121AC2DF87CCE6106E1FC07E67D4AC08A07B9B0735DFC8FA7AC1207CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527FA90A6FD991E5519A7D9FAE43ED685BB9B301AC8C09308D5A6F2FA9CAFC52D32C8325309B30B8388037FD76D11AF80A0D1CF70293F91246921DB91FC601077727EA455F16B58544A2557BDE0DD54B3590965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojm/AwjadTR3Qn+ii3DNZ8UA== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE4071D079499E9D828E9C2593EF53D4F3DC42AB412D683C4A783D51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH luajit] Emit sunk IR_NEWREF only once per key on snapshot replay. 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 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 > > > > > > Thanks to Sergey Kaplun and Peter Cawley. > > > > > > (cherry-picked from commit 1761fd2ef79ffe1778011c7e9cb03ed361b48c5e) > > > > > > +-- 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