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 73E5D6EC5A; Sat, 27 Feb 2021 01:07:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 73E5D6EC5A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1614377236; bh=hG0qS8AWUHbFYh8Mjms3yEz8sQtyRfbqFau0irZn7ZA=; 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=UKEeJogP1ymEvgGD/csJFx8B+sQOF9kA3SqCOyZdyZi4aZs8mXwN01WXfU7EdmghF dvS+t1eBVhdQGJoSH02b3NKuBdWIK8TOhfJD0XTUfZoyNuuBgEKsA0jAiPVD5aHxmT 0MR2BURWUF6bpG0nihY0sTfdHmiLHSKfnN3BjGbw= Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A82B16EC5A for ; Sat, 27 Feb 2021 01:07:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A82B16EC5A Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lFlGO-0008CP-Cv; Sat, 27 Feb 2021 01:07:08 +0300 Date: Sat, 27 Feb 2021 01:07:04 +0300 To: Sergey Kaplun Message-ID: <20210226220704.GF9042@tarantool.org> References: <20210216201044.20952-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210216201044.20952-1-skaplun@tarantool.org> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9795828B892398B728FC1D9EEA0F34691E83D75D24C38BB42182A05F538085040A76D598B6BBE21CB3820E385F9BC26CBD2BB102D8A84962EE3B33A7CDA2792EC X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B356E3E4D202B32AEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637BD378188104BC8BE8638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95CDDE882590F889B1C4FD962F02C801705718F059339A02E92A471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23DF004C9065253843057739F23D657EF2B13377AFFFEAFD26923F8577A6DFFEA7CC415C329B279CF9D93EC92FD9297F6715571747095F342E857739F23D657EF2BD5E8D9A59859A8B61F2C8E835678F328089D37D7C0E48F6C5571747095F342E857739F23D657EF2B6825BDBE14D8E7028C9DFF55498CEFB0BD9CCCA9EDD067B1EDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A555868A90C361A6CC471BA18A0EFA0E65CCF9734738D91C82D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75968C9853642EB7C3410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3438BC6CF312DA731506CEDCE72AF9071ABD186197A9BF578012408E512F291F31E09C44A6F46FEEC71D7E09C32AA3244C130D52E1D8CD5040F427D5B41F1ADBFF60759606DA2E136A927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj8mqzvzJVEn1Dr8XjwAdz7Q== X-Mailru-Sender: 689FA8AB762F73936BC43F508A06382258783717319636FA69786B111633ADB6A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] core: fix cdata decrementing 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, Thanks for the patch, nice catch! I guess we missed this bug since the tests do not cover the case for GCcdata object with a custom finalizer. But these are frequently used in Tarantool though. The changes LGTM, except several nits below. On 16.02.21, Sergey Kaplun wrote: > When cdata has custom finalizer (and so LJ_GC_CDATA_FIN flag) it is not > collected immediately, when lj_cdata_free() is called. Instead, it is > resurrected and marked finalized, so it is collected at the next GC Minor: AFAIR, the payload is released (or finalized in LuaJIT terms) at the current GC cycle. Hence, the GCobject is marked finalized at this cycle too, but it is processed once more at the next one to be released. > cycle. The reason of the bug is that gc_cdatanum is decremented when > cdata is resurrected too (i.e. twice). > > This patch excludes cdata decrementing from resurrection branch and > adds corresponding tests. > > Resolves tarantool/tarantool#5820 > Follows up tarantool/tarantool#5187 > --- > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5820-improperly-cdata-counting > Test Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5820-improperly-cdata-counting > Issue: https://github.com/tarantool/tarantool/issues/5820 > > ChangeLog entry for bumping LuaJIT: > =================================================================== > ## bugfix/LuaJIT > > * Fix double cdata decrementing in platform metrics when finalizer is set (gh-5820). > =================================================================== > > src/lj_cdata.c | 3 +- > test/misclib-getmetrics-capi.test.lua | 15 +++++++++- > test/misclib-getmetrics-capi/testgetmetrics.c | 28 +++++++++++++++++++ > test/misclib-getmetrics-lapi.test.lua | 15 +++++++++- > 4 files changed, 58 insertions(+), 3 deletions(-) > diff --git a/test/misclib-getmetrics-capi.test.lua b/test/misclib-getmetrics-capi.test.lua > index 1ad6958..e088c48 100755 > --- a/test/misclib-getmetrics-capi.test.lua > +++ b/test/misclib-getmetrics-capi.test.lua > @@ -62,6 +62,19 @@ test:ok(testgetmetrics.objcount(function(iterations) > jit.opt.start(unpack(jit_opt_default)) > end)) > > +test:ok(testgetmetrics.objcount_cdata_decrement(function() > + -- cdata decrement test. Minor: The metric name is 'gc_cdatanum', not 'cdata'. > + -- See https://github.com/tarantool/tarantool/issues/5820. > + local ffi = require("ffi") > + local function nop() end > + ffi.gc(ffi.cast("void *", 0), nop) > + -- Does not collect cdata, but removes LJ_GC_CDATA_FIN flag > + -- and resurrects object. Typo: s/resurrects object/resurrects the object/. Minor: Strictly saying, the order is a bit different: GC resurrects the object first at GCSsweep phase, and removes LJ_GC_CDATA_FIN right before invoking the finalizer at GCSfinalize phase. > + collectgarbage() > + -- Collects cdata. > + collectgarbage() > +end)) > + > -- Compiled loop with a direct exit to the interpreter. > test:ok(testgetmetrics.snap_restores(function() > jit.opt.start(0, "hotloop=1") > diff --git a/test/misclib-getmetrics-lapi.test.lua b/test/misclib-getmetrics-lapi.test.lua > index 3b3d1f8..59bcea6 100755 > --- a/test/misclib-getmetrics-lapi.test.lua > +++ b/test/misclib-getmetrics-lapi.test.lua > @@ -231,6 +231,19 @@ test:test("objcount", function(subtest) > subtest:is(new_metrics.gc_cdatanum, old_metrics.gc_cdatanum, > "cdatanum don't change") > > + -- cdata decrement test. > + -- See https://github.com/tarantool/tarantool/issues/5820. > + local function nop() end > + local cdatanum_old = misc.getmetrics().gc_cdatanum > + ffi.gc(ffi.cast("void *", 0), nop) > + -- Does not collect cdata, but removes LJ_GC_CDATA_FIN flag > + -- and resurrects object. > + collectgarbage() Side note: It's worth to check whether Tarantool internals do not spoil the counters the same way they do after making LuaJIT testing machinery self-sufficient. There is call at the beginning of the test, but I haven't tested your patch against my series. > + -- Collects cdata. > + collectgarbage() > + subtest:is(misc.getmetrics().gc_cdatanum, cdatanum_old, > + "cdatanum is decremented correctly") Minor: The metric name is 'gc_cdatanum', not 'cdatanum'. > + > -- Restore default jit settings. > jit.opt.start(unpack(jit_opt_default)) > end) > -- > 2.28.0 > -- Best regards, IM