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

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

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

Hi, Sergey,


thanks for the fixes. Please replace "grop" to "grow" in commit

message. LGTM

On 07.09.2024 08:15, Sergey Kaplun wrote:
> 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>
>

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