From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id ACA0D46970E for ; Wed, 15 Jan 2020 20:05:27 +0300 (MSK) Date: Wed, 15 Jan 2020 20:05:31 +0300 From: Alexander Turenko Message-ID: <20200115170531.zd7j43syvhp2enjw@tkn_work_nb> References: <156ce93b495648d6f3fd6c879b0d9aaf56754a1e.1574773773.git.lvasiliev@tarantool.org> <20191211222134.swzcqcbpn4tbg64m@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH] Move txn from shema to a separate module (use C API instead of FFI) List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Vasiliev Cc: tarantool-patches@dev.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.