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: Fri, 5 Mar 2021 01:04:09 +0300 [thread overview] Message-ID: <20210304220409.GK9042@tarantool.org> (raw) In-Reply-To: <20210304132832.GE6842@root> 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
next prev parent reply other threads:[~2021-03-04 22:04 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 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 [this message] 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=20210304220409.GK9042@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