Re: [Tarantool-patches] [PATCH luajit 1/2] jit: abort trace recording and execution for C API

[Igor Munkin <imun@tarantool.org>]

Vlad,

Thanks for your review!

On 03.04.20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > diff --git a/src/lj_api.c b/src/lj_api.c
> > index a5e2465..c1f53e0 100644
> > --- a/src/lj_api.c
> > +++ b/src/lj_api.c
> > @@ -300,7 +311,7 @@ LUA_API int lua_equal(lua_State *L, int idx1, int idx2)
> >        return (int)(uintptr_t)base;
> >      } else {
> >        L->top = base+2;
> > -      lj_vm_call(L, base, 1+1);
> > +      call(L, base, 1+1);
> 
> These changes look dangerous to perf. So if I had operators ==
> and < overloaded for an FFI struct, they are not jitted anymore,
> right?

Nope. I guess I need to stop right here and clarify the patch a little.

Let's start with the patch itself: the introduced changes abort the
trace recording whether re-entrancy to Lua VM is detected or "panic" if
such re-entrancy is faced while running already compiled machine code. A
good rationale for such protection is the platform failure Nikita faced
in #4427: the C function called via FFI makes the trigger with a Lua-C
API call underneath run. However it also might be any Lua-C API function
respecting the one of available metamethods (e.g. lua_less, lua_equal,
etc). If the correponding metamethod is set, the platform also re-enters
to execute it. Such behaviour is also unnacceptable, so metamethod call
ought to be invoked via jit_secure_call too. It's not optional and we
can't avoid it, since there are no guarantees the situation never
occurs.

OK, what about FFI: it's a well isolated machinery, that handles
everything its own way: own trace recording semantics, own metamethod
handling (since only tables and userdata can have an individual
metatable FFI has the one <ffi> "first level" metatable), etc. Nothing
FFI-related is changed here.

If you run the following snippet, you'll see the artefacts for
successfully compiled trace in dump.out.
| require('jit.dump').start('+tbisrmXaT', 'dump.out')
| 
| local limit = -1ULL / 2
| local value = 1ULL
| local tests = { }
| 
| while value < limit do
|   value = value * 2
|   table.insert(tests, value < limit)
| end
| 
| jit.off() -- prevent spoiling dump.out with other traces.
| print(value, limit, #tests)

Since the patch relates only to Lua-C API, metamethods triggered while
running Lua code are also successfully compiled.
| require('jit.dump').start('+tbisrmXaT', 'dump.out')
| 
| local complex
| 
| local complex_mt = {
|   __add = function(a, b) return complex(a.re + b.re, a.im + b.im) end,
|   __eq  = function(a, b) return a.re == b.re and a.im == b.im end,
|   __tostring = function(o) return ('re: %d, im: %d'):format(o.re, o.im) end
| }
| 
| complex = function(re, im)
|   return setmetatable({ re = re, im = im }, complex_mt)
| end
| 
| local start = complex(0, 0)
| local finish = complex(64, 64)
| local step = complex(1, 1)
| 
| repeat
|   start = start + step
| until start == finish

Feel free to point the parts that looks good to be included into commit
message, comments, etc.

> Why? Does someone really yield in them?

The main idea is to protect ourselves in the future (we have already
discussed it with Kostja here[1]).

> Could you measure how does it affect perf of these calls?

Talking about perf: Sergos also asked to make benchmarks and I provided
the sysbench results[2] I obtained in Gitlab. I saw no dramatically
changes. I guess the reason the performance stays the same is that
all such places has been already handled manually in Tarantool sources
(like box_txn_rollback_to_savepoint after Leonid changes in #4427).

However, if you would like to see more benchmarks, please let me know.

> From what I see, these changes basically kill FFI making it not better
> than Lua C. It no longer helps JITting anything, does it?
> 
> The same for concat, and other things. Looks like an overkill. We
> would need sandwich protection only for a few places. Is it possible
> to avoid such drastic changes?

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012732.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015236.html

-- 
Best regards,
IM