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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.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