Re: [Tarantool-patches] [PATCH 1/2] test: disable JIT for Lua Fun totable function

[Igor Munkin <imun@tarantool.org>]

Sergos,

On 25.06.20, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for explanation!
> 
> My question was about hypothetic solution - as you mentioned for a
> trace with wrong RENAME - how long will it take to implement such a
> workaround?

I'll need some time to precisely evaluate it, but for now it looks less
complex than implementing any of the approaches mentioned in the
original issue[1].

> 
> It whould affect only traces with such problem, and next approach can
> be to retry the trace with less optimizations, not just abort it?

Yes, only for the invalid traces *to be assembled*: if RENAME should be
emmited for a destructive x86 op, it would check whether there are
guarded IRs with the same snapno prior to the assembling one. Other
RENAMEs occurrences seems to be OK. After the several failures this spot
will be blacklisted and JIT will leave this route uncompiled. It's worth
to check that such drastic aborts doesn't lead to a "blacklisting
cancer".

At the same time implementing "retry the trace with less optimizations"
is way more complex than fix invalid RENAMEs emitting and ergo has no
benefits. As you said, we need a workaround until the problem is not
fixed and I guess we should aim to the least complex one here.

I'll drop all my thoughts to an issue in our GH queue (since #4252 is
closed via this patch) and will send a link here.

> 
> Regards,
> Sergos
> 
> On 21 Jun 23:24, Igor Munkin wrote:
> > Sergos,
> > 
> > On 21.06.20, Sergey Ostanevich wrote:
> > > Hi!
> > > 
> > > Thanks for the patch!
> > > 
> > > AFAIU you just fix one of our test. Is it make sence to fix the library
> > > code in fun.lua to bring the fix for all cutomers?
> > 
> > Yes, I "fixed" the test (i.e. made it non-flaky) that reproduces the
> > mentioned issue.
> > 
> > We have already discussed it offline with Sasha Tu. Of course it makes
> > sense to fix the "source" of the bug, but what exactly we need to fix?
> > 
> > Assume there is exactly the chunk in client's code making JIT to emit an
> > invalid RENAME. So there is no option to fix it in scope of the platform
> > except turning JIT off by default. The same issue is with Lua Fun: it's
> > just a "third party" Lua code leading to invalid traces assembling.
> > 
> > Furthermore, we have lots of code in the platform using Lua Fun methods
> > with not a single JIT-related problem (at least I don't know any). Lua
> > Fun is a high-performance functional programming library for Lua
> > designed with LuaJIT's trace compiler in mind (I copied this from GitHub
> > repo), ergo disabling JIT for the whole library will nerf performance
> > for the well-compiled Lua code.
> > 
> > As a result I see two solutions (besides fixing the root cause):
> > * Turn JIT engine off for the whole platform (or even disable JIT
> >   machinery at all). It definitely will lead to dramatic performance
> >   drop and might also lead to further dramatic source code changes,
> >   since some Tarantool parts are tuned a lot regarding JIT specifics.
> > * Provide a recipe for ones facing the similar problem, so they can use
> >   Lua Fun until the fix is ready. I guess the way the patch is done can
> >   be shared with all customers.
> > 
> > > 
> > > Also, as soon as we know the #584 essence - how hard will it be to test
> > > all Tarantool libs for this bug?
> > 
> > Since JIT tries to optimize and narrow Lua dynamic specifics, it's like
> > finding a needle in a haystack. As you can see in my reproducer[1] I
> > just reduce register pressure via enabling sink optimization and amiss
> > RENAME is emitted. With disabled sink optimization everything works
> > fine.
> > 
> > It would be nice to enrich our suite with such tests for other Tarantool
> > libs, but even this single case is way too complex to find: we need to
> > emit destructive x86 op with the different "source" and "destination"
> > registers allocated between two side exits related to the same snapshot
> > -- I'm not sure I didn't missed something.
> > 
> > It looks like we can add a trace abort (or even assert) for a RENAME
> > emitted between two side exits with the same snapno. As a result broken
> > traces are not compiled. Thoughts? I need some time to think about it.
> > 
> > Long story short: the stars were aligned and the failure occurred. For
> > now I have no idea how to catch such traces without patching assembling
> > machinery.
> > 
> > > 
> > > Regards,
> > > Sergos
> > > 
> > > 
> > > On 19 Jun 23:40, Igor Munkin wrote:
> > > > JIT compiler can generate invalid trace for <totable> function breaking
> > > > its semantics (see LuaJIT/LuaJIT#584). Since interpreter works fine and
> > > > produces right results, disabling JIT for this function (and all its
> > > > subfunctions) stops execution failures.
> > > > 
> > > > Relates to LuaJIT/LuaJIT#584
> > > > Fixes #4252
> > > > 
> > > > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > > > ---
> > > >  test/box-tap/key_def.test.lua | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua
> > > > index d7dbf5b88..ce7a3cb63 100755
> > > > --- a/test/box-tap/key_def.test.lua
> > > > +++ b/test/box-tap/key_def.test.lua
> > > > @@ -4,6 +4,14 @@ local tap = require('tap')
> > > >  local ffi = require('ffi')
> > > >  local json = require('json')
> > > >  local fun = require('fun')
> > > > +
> > > > +-- Fix for gh-4252: to prevent invalid trace assembling (see
> > > > +-- LuaJIT/LuaJIT#584) disable JIT for fun.totable function and
> > > > +-- method (these functions are different GCfunc objects) and all
> > > > +-- their subfunctions.
> > > > +jit.off(fun.totable, true)
> > > > +jit.off(fun.iter({}).totable, true)
> > > > +
> > > >  local key_def_lib = require('key_def')
> > > >  
> > > >  local usage_error = 'Bad params, use: key_def.new({' ..
> > > > -- 
> > > > 2.25.0
> > > > 
> > 
> > [1]: https://gist.github.com/igormunkin/18cb6afe5a495bce31f772d453f3117e#file-4252-lua
> > 
> > -- 
> > Best regards,
> > IM

-- 
Best regards,
IM