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 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-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