From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit] core: fix cdata decrementing Date: Sat, 27 Feb 2021 01:07:04 +0300 [thread overview] Message-ID: <20210226220704.GF9042@tarantool.org> (raw) In-Reply-To: <20210216201044.20952-1-skaplun@tarantool.org> 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(-) <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)) > 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") <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, > "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 <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, > + "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
next prev parent reply other threads:[~2021-02-26 22:07 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-16 20:10 Sergey Kaplun via Tarantool-patches 2021-02-26 22:07 ` Igor Munkin via Tarantool-patches [this message] 2021-03-02 9:27 ` Sergey Ostanevich via Tarantool-patches 2021-03-04 13:35 ` Sergey Kaplun via Tarantool-patches 2021-03-04 13:28 ` Sergey Kaplun via Tarantool-patches 2021-03-04 22:04 ` Igor Munkin via Tarantool-patches 2021-03-04 22:04 ` Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210226220704.GF9042@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit] core: fix cdata decrementing' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox