[Tarantool-patches] [PATCH luajit] core: fix cdata decrementing

[Tarantool-patches] [PATCH luajit] core: fix cdata decrementing

[Sergey Kaplun via Tarantool-patches]

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
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/src/lj_cdata.c b/src/lj_cdata.c
index 0dd8553..d3042f2 100644
--- a/src/lj_cdata.c
+++ b/src/lj_cdata.c
@@ -80,10 +80,11 @@ void LJ_FASTCALL lj_cdata_free(global_State *g, GCcdata *cd)
     lua_assert(ctype_hassize(ct->info) || ctype_isfunc(ct->info) ||
 	       ctype_isextern(ct->info));
     lj_mem_free(g, cd, sizeof(GCcdata) + sz);
+    g->gc.cdatanum--;
   } else {
     lj_mem_free(g, memcdatav(cd), sizecdatav(cd));
+    g->gc.cdatanum--;
   }
-  g->gc.cdatanum--;
 }
 
 void lj_cdata_setfin(lua_State *L, GCcdata *cd, GCobj *obj, uint32_t it)
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
@@ -14,7 +14,7 @@ local jit_opt_default = {
 local tap = require('tap')
 
 local test = tap.test("clib-misc-getmetrics")
-test:plan(10)
+test:plan(11)
 
 local testgetmetrics = require("testgetmetrics")
 
@@ -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.
+    -- 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.
+    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-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
index 7fd3eef..6777633 100644
--- a/test/misclib-getmetrics-capi/testgetmetrics.c
+++ b/test/misclib-getmetrics-capi/testgetmetrics.c
@@ -155,6 +155,33 @@ static int objcount(lua_State *L)
 	return 1;
 }
 
+static int objcount_cdata_decrement(lua_State *L)
+{
+	/*
+	 * cdata decrement test.
+	 * See https://github.com/tarantool/tarantool/issues/5820.
+	 */
+	struct luam_Metrics oldm, newm;
+	int n = lua_gettop(L);
+	if (n != 1 || !lua_isfunction(L, 1))
+		luaL_error(L, "incorrect argument: 1 function is required");
+
+	/* Force up garbage collect all dead objects. */
+	lua_gc(L, LUA_GCCOLLECT, 0);
+
+	luaM_metrics(L, &oldm);
+	/*
+	 * The function generates and collects cdata with
+	 * LJ_GC_CDATA_FIN flag.
+	 */
+	lua_call(L, 0, 0);
+	luaM_metrics(L, &newm);
+	assert(newm.gc_cdatanum - oldm.gc_cdatanum == 0);
+
+	lua_pushboolean(L, 1);
+	return 1;
+}
+
 static int snap_restores(lua_State *L)
 {
 	struct luam_Metrics oldm, newm;
@@ -229,6 +256,7 @@ static const struct luaL_Reg testgetmetrics[] = {
 	{"gc_allocated_freed", gc_allocated_freed},
 	{"gc_steps", gc_steps},
 	{"objcount", objcount},
+	{"objcount_cdata_decrement", objcount_cdata_decrement},
 	{"snap_restores", snap_restores},
 	{"strhash", strhash},
 	{"tracenum_base", tracenum_base},
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
@@ -172,7 +172,7 @@ test:test("gc-steps", function(subtest)
 end)
 
 test:test("objcount", function(subtest)
-    subtest:plan(4)
+    subtest:plan(5)
     local ffi = require("ffi")
 
     jit.opt.start(0)
@@ -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()
+    -- Collects cdata.
+    collectgarbage()
+    subtest:is(misc.getmetrics().gc_cdatanum, cdatanum_old,
+               "cdatanum is decremented correctly")
+
     -- Restore default jit settings.
     jit.opt.start(unpack(jit_opt_default))
 end)
-- 
2.28.0

Re: [Tarantool-patches] [PATCH luajit] core: fix cdata decrementing

[Igor Munkin via 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(-)

<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

Re: [Tarantool-patches] [PATCH luajit] core: fix cdata decrementing

[Sergey Ostanevich via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 5311 bytes --]

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


[-- Attachment #2: Type: text/html, Size: 37969 bytes --]

Re: [Tarantool-patches] [PATCH luajit] core: fix cdata decrementing

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!

Thanks for the review!

Added your tag:
| Reviewed-by: Igor Munkin <imun@tarantool.org>

On 27.02.21, 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.

Yes, as you said below: it is made white (is resurrected) in sweep phase
and finalized additional payload in GCfinalize phase. The cdata
object is collected at the next GC cycle.

I haven't change commit message since it means the same. Please
correct me if I'm missing something. You may update this commit
message in way what you want during cherry picking.

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

Fixed, branch is force pushed. See the itterative patch below.
===================================================================
diff --git a/test/misclib-getmetrics-capi.test.lua b/test/misclib-getmetrics-capi.test.lua
index e088c48..5c0e659 100755
--- a/test/misclib-getmetrics-capi.test.lua
+++ b/test/misclib-getmetrics-capi.test.lua
@@ -63,15 +63,15 @@ test:ok(testgetmetrics.objcount(function(iterations)
 end))
 
 test:ok(testgetmetrics.objcount_cdata_decrement(function()
-    -- cdata decrement test.
+    -- gc_cdatanum decrement test.
     -- 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.
+    -- Does not collect the cdata, but resurrects the object and
+    -- removes LJ_GC_CDATA_FIN flag.
     collectgarbage()
-    -- Collects cdata.
+    -- Collects the cdata.
     collectgarbage()
 end))
 
diff --git a/test/misclib-getmetrics-lapi.test.lua b/test/misclib-getmetrics-lapi.test.lua
index 59bcea6..0d054d3 100755
--- a/test/misclib-getmetrics-lapi.test.lua
+++ b/test/misclib-getmetrics-lapi.test.lua
@@ -231,15 +231,15 @@ test:test("objcount", function(subtest)
     subtest:is(new_metrics.gc_cdatanum, old_metrics.gc_cdatanum,
                "cdatanum don't change")
 
-    -- cdata decrement test.
+    -- gc_cdatanum 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.
+    -- Does not collect the cdata, but resurrects the object and
+    -- removes LJ_GC_CDATA_FIN flag.
     collectgarbage()
-    -- Collects cdata.
+    -- Collects the cdata.
     collectgarbage()
     subtest:is(misc.getmetrics().gc_cdatanum, cdatanum_old,
                "cdatanum is decremented correctly")
===================================================================

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

I've checked it at the new branch version:
https://github.com/tarantool/tarantool/tree/skaplun/gh-5820-improperly-cdata-counting
Looks OK.

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

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] core: fix cdata decrementing

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!
Added your tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

On 02.03.21, Sergey Ostanevich wrote:
> 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?

I've checked these chunks again:

| $ grep lj_mem_newgco -R ../src | grep GC | grep -v -e ircall -e FASTCALL
| ../src/lj_parse.c:  pt = (GCproto *)lj_mem_newgco(L, (MSize)sizept);
| ../src/lj_bcread.c:  pt = (GCproto *)lj_mem_newgco(ls->L, (MSize)sizept);
| ../src/lj_tab.c:    t = (GCtab *)lj_mem_newgco(L, sizetabcolo(asize));
| ../src/lj_func.c:  GCupval *uv = (GCupval *)lj_mem_newgco(L, sizeof(GCupval));
| ../src/lj_func.c:  GCfunc *fn = (GCfunc *)lj_mem_newgco(L, sizeCfunc(nelems));
| ../src/lj_func.c:  GCfunc *fn = (GCfunc *)lj_mem_newgco(L, sizeLfunc((MSize)pt->sizeuv));
| ../src/lj_cdata.h:  cd = (GCcdata *)lj_mem_newgco(cts->L, sizeof(GCcdata) + sz);
| ../src/lj_cdata.h:  GCcdata *cd = (GCcdata *)lj_mem_newgco(L, sizeof(GCcdata) + sz);

We count tabs and cdata correctly.
As for prototypes, upvalues and functions counters -- they are not
reported by design for now.

> 
> regards,
> Sergos
> 

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] core: fix cdata decrementing

[Igor Munkin via Tarantool-patches]

Sergey,

On 04.03.21, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the review!
> 
> Added your tag:
> | Reviewed-by: Igor Munkin <imun@tarantool.org>
> 
> On 27.02.21, 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.
> 
> Yes, as you said below: it is made white (is resurrected) in sweep phase
> and finalized additional payload in GCfinalize phase. The cdata
> object is collected at the next GC cycle.
> 
> I haven't change commit message since it means the same. Please
> correct me if I'm missing something. You may update this commit
> message in way what you want during cherry picking.

Sorry, you're right and v buhgalterii vse pereputali. I left the commit
message unchanged.

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

> > 
> > 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.
> 
> I've checked it at the new branch version:
> https://github.com/tarantool/tarantool/tree/skaplun/gh-5820-improperly-cdata-counting
> Looks OK.

Yes, CI is green and this is great. Thanks for checking!

> 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] core: fix cdata decrementing

[Igor Munkin via Tarantool-patches]

Sergey,

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
> 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).
> ===================================================================

I've changed the ChangeLog entry a bit. Hope, you don't mind.

================================================================================

## bugfix/LuaJIT

* Fixed double `gc_cdatanum` decrementing in LuaJIT platform metrics when a
  finalizer is set for GCcdata object (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(-)

I've checked the patchset into tarantool-2.6, tarantool-2.7 and
tarantool branches in tarantool/luajit and bumped a new version in 2.6,
2.7 and master.

> 

<snipped>

> -- 
> 2.28.0
> 

-- 
Best regards,
IM