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

[Igor Munkin <imun@tarantool.org>]

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