Tarantool development patches archive
 help / color / mirror / Atom feed
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

  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