Re: [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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


Hi, Sergey!
Thanks for the patch!
LGTM
 
--
Best regards,
Maxim Kokryashkin
 
  
>Вторник, 23 января 2024, 16:18 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Sergey!
>Thanks for the review!
>
>On 23.01.24, Sergey Bronnikov wrote:
>> Hi, Sergey!
>>
>>
>> thanks for the patch! LGTM with a minor comment below
>>
>>
>> Sergey
>>
>>
>> On 1/22/24 12:32, Sergey Kaplun wrote:
>> > From: Mike Pall <mike>
>> >
>> > Thanks to Sergey Kaplun.
>> >
>> > (cherry-picked from commit 7dbe545933485849977d50384f2f20f2cccf0cf9)
>> >
>> > Before this commit, the JIT engine didn't check the status of JIT flags
>> > when compiling the side trace. Hence, after calling `jit.off()` the side
>> > traces are still recorded. This patch adds the aforementioned check.
>> >
>> > Sergey Kaplun:
>> > * added the description and the test for the problem
>> >
>> > Part of tarantool/tarantool#9595
>> > ---
>> >
>> > Branch:  https://github.com/tarantool/luajit/tree/skaplun/lj-1134-hotside-jit-off
>> > Tarantool PR:  https://github.com/tarantool/tarantool/pull/9607
>> > Related issues:
>> > *  https://github.com/tarantool/tarantool/issues/9595
>> > *  https://github.com/LuaJIT/LuaJIT/issues/1134
>> >
>> > src/lj_trace.c | 2 +-
>> > .../lj-1134-hotside-jit-off.test.lua | 44 +++++++++++++++++++
>> > 2 files changed, 45 insertions(+), 1 deletion(-)
>> > create mode 100644 test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
>> >
>> > diff --git a/src/lj_trace.c b/src/lj_trace.c
>> > index 236e06a0..20014ecb 100644
>> > --- a/src/lj_trace.c
>> > +++ b/src/lj_trace.c
>> > @@ -917,7 +917,7 @@ int LJ_FASTCALL lj_trace_exit(jit_State *J, void *exptr)
>> > } else if (G(L)->gc.state == GCSatomic || G(L)->gc.state == GCSfinalize) {
>> > if (!(G(L)->hookmask & HOOK_GC))
>> > lj_gc_step(L); /* Exited because of GC: drive GC forward. */
>> > - } else {
>> > + } else if ((J->flags & JIT_F_ON)) {
>> > trace_hotside(J, pc);
>> > }
>> > if (bc_op(*pc) == BC_JLOOP) {
>> > diff --git a/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
>> > new file mode 100644
>> > index 00000000..cdee3eb2
>> > --- /dev/null
>> > +++ b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
>> > @@ -0,0 +1,44 @@
>> > +local tap = require('tap')
>> > +
>> > +-- Test file to demonstrate the JIT misbehaviour, when the side
>> > +-- trace is compiled after `jit.off()`.
>> > +-- See also:  https://github.com/LuaJIT/LuaJIT/issue/1134 .
>> > +
>> > +local test = tap.test('lj-1134-hotside-jit-off'):skipcond({
>> > + ['Test requires JIT enabled'] = not jit.status(),
>> > + ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>> > +})
>> > +
>> > +local traceinfo = require('jit.util').traceinfo
>> > +
>> > +test:plan(1)
>> > +
>> > +local take_side
>> > +local function trace()
>> > + -- luacheck: ignore
>> > + -- Branch for the side exit.
>> > + if take_side then end
>> > +end
>> > +
>> > +-- Flush all possible traces.
>> > +jit.flush()
>> > +
>> > +jit.opt.start('hotloop=1', 'hotexit=1')
>> > +
>> > +trace()
>> > +trace()
>> > +
>> > +assert(traceinfo(1), 'root trace not compiled')
>>
>> It is not clear what "1" means (here and below). As I got it right, it
>> is a trace number.
>>
>> I would left a comment about magic number or replace constant with
>> variable with self-explained name.
>
>Added the following comment. Branch is force-pushed.
>
>===================================================================
>diff --git a/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
>index cdee3eb2..080b1e87 100644
>--- a/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
>+++ b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
>@@ -9,6 +9,7 @@ local test = tap.test('lj-1134-hotside-jit-off'):skipcond({
>   ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> })
> 
>+-- `traceinfo()` takes the trace number as an argument.
> local traceinfo = require('jit.util').traceinfo
> 
> test:plan(1)
>===================================================================
>
>>
>> > +
>> > +-- Force trace exit.
>> > +take_side = true
>> > +
>> > +jit.off()
>> > +
>> > +-- Attempt to compile a side trace.
>> > +trace()
>> > +trace()
>> > +
>> > +test:ok(not traceinfo(2), 'no side trace compiled')
>> > +
>> > +test:done(true)
>
>--
>Best regards,
>Sergey Kaplun
 

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