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

[Leonid Vasiliev <lvasiliev@tarantool.org>]

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?