* [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit. @ 2024-01-22 9:32 Sergey Kaplun via Tarantool-patches 2024-01-23 8:33 ` Sergey Bronnikov via Tarantool-patches 2024-02-15 13:43 ` Igor Munkin via Tarantool-patches 0 siblings, 2 replies; 6+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-01-22 9:32 UTC (permalink / raw) To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches 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') + +-- 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) -- 2.43.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit. 2024-01-22 9:32 [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit Sergey Kaplun via Tarantool-patches @ 2024-01-23 8:33 ` Sergey Bronnikov via Tarantool-patches 2024-01-23 13:14 ` Sergey Kaplun via Tarantool-patches 2024-02-15 13:43 ` Igor Munkin via Tarantool-patches 1 sibling, 1 reply; 6+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-01-23 8:33 UTC (permalink / raw) To: Sergey Kaplun, Maxim Kokryashkin; +Cc: tarantool-patches 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. > + > +-- 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) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit. 2024-01-23 8:33 ` Sergey Bronnikov via Tarantool-patches @ 2024-01-23 13:14 ` Sergey Kaplun via Tarantool-patches 2024-01-25 7:49 ` Sergey Bronnikov via Tarantool-patches 2024-01-29 11:39 ` Maxim Kokryashkin via Tarantool-patches 0 siblings, 2 replies; 6+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-01-23 13:14 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit. 2024-01-23 13:14 ` Sergey Kaplun via Tarantool-patches @ 2024-01-25 7:49 ` Sergey Bronnikov via Tarantool-patches 2024-01-29 11:39 ` Maxim Kokryashkin via Tarantool-patches 1 sibling, 0 replies; 6+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-01-25 7:49 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches On 1/23/24 16:14, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the review! <snipped> > + > +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. Thanks! LGTM as I said before > > =================================================================== > 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) > =================================================================== > <snipped> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit. 2024-01-23 13:14 ` Sergey Kaplun via Tarantool-patches 2024-01-25 7:49 ` Sergey Bronnikov via Tarantool-patches @ 2024-01-29 11:39 ` Maxim Kokryashkin via Tarantool-patches 1 sibling, 0 replies; 6+ messages in thread From: Maxim Kokryashkin via Tarantool-patches @ 2024-01-29 11:39 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit. 2024-01-22 9:32 [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit Sergey Kaplun via Tarantool-patches 2024-01-23 8:33 ` Sergey Bronnikov via Tarantool-patches @ 2024-02-15 13:43 ` Igor Munkin via Tarantool-patches 1 sibling, 0 replies; 6+ messages in thread From: Igor Munkin via Tarantool-patches @ 2024-02-15 13:43 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches Sergey, I've checked the patchset into all long-term branches in tarantool/luajit and bumped a new version in master, release/3.0 and release/2.11. On 22.01.24, Sergey Kaplun via Tarantool-patches 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 > <snipped> > -- > 2.43.0 > -- Best regards, IM ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-15 13:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-01-22 9:32 [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit Sergey Kaplun via Tarantool-patches 2024-01-23 8:33 ` Sergey Bronnikov via Tarantool-patches 2024-01-23 13:14 ` Sergey Kaplun via Tarantool-patches 2024-01-25 7:49 ` Sergey Bronnikov via Tarantool-patches 2024-01-29 11:39 ` Maxim Kokryashkin via Tarantool-patches 2024-02-15 13:43 ` Igor Munkin via Tarantool-patches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox