Re: [Tarantool-patches] [PATCH] Move txn from shema to a separate module (use C API instead of FFI)

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Thu, Dec 12, 2019 at 11:23:14AM +0300, Leonid Vasiliev wrote:
> 
> 
> On 12/12/19 1:21 AM, Alexander Turenko wrote:
> > Kostya, thanks for bringing the FFI / Lua-C question into our view.
> > 
> > ----
> > 
> > We decided to don't change existing convention: let's use FFI when
> > possible and Lua-C API for functions that access a Lua state somehow
> > (including touching tarantool_L, yielding and running triggers /
> > callbacks).
> > 
> > The core reason is that FFI allows to produce longer JIT traces that
> > should positively impact performance of an application at whole.
> > 
> > We have no good understanding how performant Lua-C API with trace
> > stitching, however simple benchmarks (shared by Leonid) shows positive
> > impact of using FFI calls.
> > 
> > We have no interpretation for those benchmarks for now. Whether Lua-C
> > runs were JITed at all? Whether a result will be different if there will
> > be more JITable code around? So for now things looks just as 'FFI is
> > faster the Lua-C'. Hope we'll investigate this later.
> > 
> > We need to find a way to reduce probability that a function that is
> > called via ffi (or one that is called from it) will be changed in a
> > future and starts to yield or touch a Lua state.
> > 
> > We should look whether we can mark functions that are called from ffi
> > (see also [1]) and all its callee to at least verify ourself when
> > changing code.
> > 
> > Also we can work on detection of so called ffi sandwitches and doing
> > more stress testing.
> > 
> > It seems that both directions are worth to dig into.
> > 
> > [1]: https://github.com/tarantool/tarantool/issues/4202
> > 
> > ----
> > 
> > Back to the patch.
> > 
> > Since we'll just move one function, I would not perform refactoring I
> > proposed initially: don't move all related functions together. Let's
> > rewrite box.rollback_to_savepoint() using Lua-C API and place it together
> > with box.commit() and box.rollback() to src/box/lua/init.c >
> > I think we don't need any pure Lua wrapper around this function: we can
> > access a Lua table field and raise an error just from C, so I don't see
> > a reason to split a function logic around two files
> > (src/box/lua/schema.lua and src/box/lua/init.c). They are both about API
> > we expose to Lua.
> > 
> > WBR, Alexander Turenko.
> >  > On Tue, Nov 26, 2019 at 04:13:43PM +0300, Leonid wrote:
> > > https://github.com/tarantool/tarantool/issues/4427
> > > https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4427-move-some-stuff-from-ffi-to-c-api
> > > 
> > > ---
> > >   src/box/CMakeLists.txt       |   2 +
> > >   src/box/lua/init.c           |   4 +
> > >   src/box/lua/schema.lua       |  71 -----------------
> > >   src/box/lua/txn.c            | 145 +++++++++++++++++++++++++++++++++++
> > >   src/box/lua/txn.h            |  49 ++++++++++++
> > >   src/box/lua/txn.lua          |  53 +++++++++++++
> > >   src/box/txn.c                |   2 +
> > >   test/box/misc.result         |   1 +
> > >   test/engine/savepoint.result |  12 +--
> > >   9 files changed, 262 insertions(+), 77 deletions(-)
> > >   create mode 100644 src/box/lua/txn.c
> > >   create mode 100644 src/box/lua/txn.h
> > >   create mode 100644 src/box/lua/txn.lua
> > 
> > > --- box.commit yields, so it's defined as Lua/C binding
> > > --- box.rollback yields as well
> > 
> > Let's move box.rollback_to_savepoint() to them.
> > 
> 
> I think to have a separate module for txn is a good idea. IMHO a true way
> use FFI for begin and is_in_txn and use C API for create a savepoint and
> rollback. Really, it's more consistent in my mind. What do you think about
> it?

Personally I'm not sure that this refactoring is valuable enough to do
it. I don't mind, however: let's propose it after we'll fix the bug, if
you want.

But I think that creation of a savepoint using Lua-C violaves the rule
re FFI / Lua-C API usage (cited from above):

> Use FFI when possible and Lua-C API for functions that access a Lua
> state somehow (including touching tarantool_L, yielding and running
> triggers / callbacks).

I would maybe add an exception for some pure Lua-C API modules (I wrote
about it to the related internal mail thread, see '[dev] RFC: FFI usage
guidance'), but this is not our case. Moving a function from FFI to
Lua-C should be strictly motivated. Since we're working on a strict rule
and your proposal violates the one cited above, can you propose a
wording that will handle your case gracefully?

I'll look into the patch, which moves just one function ('txn: move
rollback to savepoint from ffi to C-API' from 2019-12-13) soon.

WBR, Alexander Turenko.