Re: [Tarantool-patches] [PATCH luajit] FFI: Drop finalizer table rehash after GC cycle.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!
Fixed your comments, rebased the branch on the tarantool/master and
force-pushed it.

On 06.09.24, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> 
> thanks for the patch! LGTM with minor comments below
> 
> On 02.09.2024 15:54, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Reported by Sergey Kaplun.
> >
> > (cherry picked from commit fb22d0f80f291827a4004e16bc589b54bcc4a3c7)
> >
> > The raising of the OOM error when rehashing the finalizer table (when we
> > can't allocate a new hash part) leads to crashes in either
> > `lj_trace_exit()` or `lj_trace_unwind()` due to unprotected error
> > raising, which either has no DWARF eh_frame or loses the context of the
> 
> I would add a link to a page about eh_frame, for example [1]
> 
> Feel free to ignore.
> 
> 1. 
> https://refspecs.linuxfoundation.org/LSB_3.0.0/LSB-Core-generic/LSB-Core-generic/ehframechpt.html

Added, thanks.

> 
> > JIT compiler.
> >
> > This patch drops rehashing of the finalizer table to avoid these
> 
> I would replace "finalizer" with "cdata finalizer".
> 
> And I would say about drawbacks of this. Otherwise, it looks like

Added.
See the new commit message below:

| FFI: Drop finalizer table rehash after GC cycle.
|
| Reported by Sergey Kaplun.
|
| (cherry picked from commit fb22d0f80f291827a4004e16bc589b54bcc4a3c7)
|
| The raising of the OOM error when rehashing the finalizer table (when we
| can't allocate a new hash part) leads to crashes in either
| `lj_trace_exit()` or `lj_trace_unwind()` due to unprotected error
| raising, which either has no DWARF eh_frame [1] or loses the context of
| the JIT compiler.
|
| This patch drops rehashing of the cdata finalizer table to avoid these
| crashes. It will prevent the cdata finalizer table from shrinking when
| the huge amount of the cdata objects is collected by the GC. OTOH, the
| finzlizer table most probably will grop anyway to the old size, so this
| is not crucial.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| [1]: https://refspecs.linuxfoundation.org/LSB_3.0.0/LSB-Core-generic/LSB-Core-generic/ehframechpt.html
|
| Part of tarantool/tarantool#10199
| Resolves tarantool/tarantool#10290

> 
> rehashing was not needed from the beginning.
> 
> > crashes.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#10199
> > Resolves tarantool/tarantool#10290
> Usually "Closes" or "Fixes". Feel free to ignore.

For LuaJIT, we always use "Resolves", since it is __closed__ when the
submodule is bumped in the release branches and master.

> > ---
> >
> > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1247-fin-tab-rehashing-on-trace
> > Related Issues:
> > *https://github.com/tarantool/tarantool/issues/10290
> > *https://github.com/LuaJIT/LuaJIT/issues/1247
> > *https://github.com/tarantool/tarantool/issues/10199
> >

<snipped>

-- 
Best regards,
Sergey Kaplun