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 5F2916EC5F; Tue, 2 Mar 2021 12:27:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5F2916EC5F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1614677256; bh=ZAoHleRzWUrtOTLqCFLge9JCl132Wu/XxR411cfcUao=; h=Date:In-Reply-To:To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=zYzcZNQKBcNvHFRYNbH/HiIIM8NxA5a+HjCoo5ETC6a9uD4wAoT1w6mlT2yIai42/ zvPhA/JWW5TJf1LGXxN4qNVrFDCQueIwnJw/h0/whf2PBxx+3AitG8dvjwrCxAY/Ci cYDQZ6G8BSrWr5g95G7HOoUuVZyHgTUjgqIqLVQM= Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 E4C5D6EC5F for ; Tue, 2 Mar 2021 12:27:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E4C5D6EC5F Received: by smtp42.i.mail.ru with esmtpa (envelope-from ) id 1lH1JW-0002uX-3A; Tue, 02 Mar 2021 12:27:34 +0300 Message-Id: <790A703F-AB21-4C0C-8178-DC85E8336B2A@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_D7E4C6B0-6F03-49B7-AF3E-E3DA4259E2FF" Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.60.0.2.21\)) Date: Tue, 2 Mar 2021 12:27:33 +0300 In-Reply-To: <20210226220704.GF9042@tarantool.org> To: Igor Munkin References: <20210216201044.20952-1-skaplun@tarantool.org> <20210226220704.GF9042@tarantool.org> X-Mailer: Apple Mail (2.3654.60.0.2.21) X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD92A98208ECBDD29F51A014079BC0C7C459F7160AC7246F773182A05F538085040AAD1AE6F2B22EC70C098F0B38FB7894AE2FC8C354514C124E806B623610F59E5 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72267471453D8B600EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637F0135404761DA3FC8638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FCE5411627659F8B2B479C65F8CDEAD1BE645FB8D4EEE4D91A389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B64854413538E1713FCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249C97EC07E32150B0F76E601842F6C81A12EF20D2F80756B5F7E9C4E3C761E06A776E601842F6C81A127C277FBC8AE2E8BA148C6F4312824303AA81AA40904B5D9DBF02ECDB25306B2B25CBF701D1BE8734AD6D5ED66289B5278DA827A17800CE702706FBA1021170467F23339F89546C5A8DF7F3B2552694A6FED454B719173D6725E5C173C3A84C35D1D84EF68E022EA35872C767BF85DA2F004C906525384306FED454B719173D6462275124DF8B9C9DF33B08B2BB81206574AF45C6390F7469DAA53EE0834AAEE X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A24A6D60772A99906F8E1CD14B953EB46D970F3B3AEA0C0B52355D89D7DBCDD132 X-C1DE0DAB: 0D63561A33F958A5A53178B842585D8708507BCA53F30204F14185FE5CA8033BD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D340A59E724FC7897F7DCD3C14F9782ACF5AC58E3DB9570B94218507CEF32B67A4B8B5E421AFFA3E12A1D7E09C32AA3244C4E296F3656E13F4E0A5598D55CA576A8A90944CA99CF22E3FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnpKwxR6GFbsesucSjwF3nA== X-Mailru-Sender: 455D65AE3A139168626D8C76E86D3AC09CF153CB1C1B02B404A7DBB86F3461EDDDAC019A3742A8B676D79013C85012CDC77752E0C033A69E4BBE7EBD99111A499D0AB74157175C036C18EFA0BB12DBB0 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: Sergey Ostanevich via Tarantool-patches Reply-To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" --Apple-Mail=_D7E4C6B0-6F03-49B7-AF3E-E3DA4259E2FF Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii Hi! Thanks for the patch! LGTM on the changes, still I can see a number of places you do not=20 collect information about GC objects being created, although they all about function parsing/creating.=20 Are we intentionally not looking into these? regards, Sergos > On 27 Feb 2021, at 01:07, Igor Munkin wrote: >=20 > Sergey, >=20 > 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. >=20 > 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 >=20 > 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. >=20 >> cycle. The reason of the bug is that gc_cdatanum is decremented when >> cdata is resurrected too (i.e. twice). >>=20 >> This patch excludes cdata decrementing from resurrection branch and >> adds corresponding tests. >>=20 >> Resolves tarantool/tarantool#5820 >> Follows up tarantool/tarantool#5187 >> --- >>=20 >> 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-cda= ta-counting >> Issue: https://github.com/tarantool/tarantool/issues/5820 >>=20 >> ChangeLog entry for bumping LuaJIT: >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> ## bugfix/LuaJIT >>=20 >> * Fix double cdata decrementing in platform metrics when finalizer is = set (gh-5820). >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>=20 >> 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(-) >=20 > >=20 >> 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 >=20 > >=20 >> @@ -62,6 +62,19 @@ = test:ok(testgetmetrics.objcount(function(iterations) >> jit.opt.start(unpack(jit_opt_default)) >> end)) >>=20 >> +test:ok(testgetmetrics.objcount_cdata_decrement(function() >> + -- cdata decrement test. >=20 > Minor: The metric name is 'gc_cdatanum', not 'cdata'. >=20 >> + -- See https://github.com/tarantool/tarantool/issues/5820 = . >> + local ffi =3D 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. >=20 > Typo: s/resurrects object/resurrects the object/. >=20 > 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. >=20 >> + 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=3D1") >=20 > >=20 >> 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 >=20 > >=20 >> @@ -231,6 +231,19 @@ test:test("objcount", function(subtest) >> subtest:is(new_metrics.gc_cdatanum, old_metrics.gc_cdatanum, >> "cdatanum don't change") >>=20 >> + -- cdata decrement test. >> + -- See https://github.com/tarantool/tarantool/issues/5820 = . >> + local function nop() end >> + local cdatanum_old =3D 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() >=20 > 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. >=20 >> + -- Collects cdata. >> + collectgarbage() >> + subtest:is(misc.getmetrics().gc_cdatanum, cdatanum_old, >> + "cdatanum is decremented correctly") >=20 > Minor: The metric name is 'gc_cdatanum', not 'cdatanum'. >=20 >> + >> -- Restore default jit settings. >> jit.opt.start(unpack(jit_opt_default)) >> end) >> --=20 >> 2.28.0 >>=20 >=20 > --=20 > Best regards, > IM --Apple-Mail=_D7E4C6B0-6F03-49B7-AF3E-E3DA4259E2FF Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii Hi!

Thanks = for the patch!
LGTM on the changes, still I can see = a number of places you do not 
collect = information about GC objects being created, although
they all about function parsing/creating. 

Are we intentionally not = looking into these?

regards,
Sergos



On 27 Feb 2021, at 01:07, Igor Munkin <imun@tarantool.org> = wrote:

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-improp= erly-cdata-counting
Test Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5820-imp= roperly-cdata-counting
Issue: https://github.com/tarantool/tarantool/issues/5820

ChangeLog entry for bumping LuaJIT:
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
## bugfix/LuaJIT

* Fix double = cdata decrementing in platform metrics when finalizer is set = (gh-5820).
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

src/lj_cdata.c =             &n= bsp;           &nbs= p;      |  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(-)

<snipped>

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

<snipped>

@@ -62,6 +62,19 @@ = test:ok(testgetmetrics.objcount(function(iterations)
    jit.opt.start(unpack(jit_opt_default))<= br class=3D"">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 =3D 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=3D1")

<snipped>

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

<snipped>

@@ -231,6 +231,19 @@ = test:test("objcount", function(subtest)
    subtest:is(new_metrics.gc_cdatanum, = old_metrics.gc_cdatanum,
          &nb= sp;    "cdatanum don't change")

+    -- cdata decrement test.
+ =    -- See https://github.com/tarantool/tarantool/issues/5820.
+    local function nop() end
+ =    local cdatanum_old =3D = 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 <collectgarbage> 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,
+ =             &n= bsp; "cdatanum is decremented correctly")

Minor: The metric name is 'gc_cdatanum', not = 'cdatanum'.

+
    -- Restore default jit settings.
    jit.opt.start(unpack(jit_opt_default))<= br class=3D"">end)
-- 
2.28.0


-- Best = regards,
IM

= --Apple-Mail=_D7E4C6B0-6F03-49B7-AF3E-E3DA4259E2FF--