From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 4AB4C4696C3 for ; Sat, 4 Apr 2020 15:01:26 +0300 (MSK) Date: Sat, 4 Apr 2020 14:55:05 +0300 From: Igor Munkin Message-ID: <20200404115505.GD30022@tarantool.org> References: <50fe58f83ebb4e4971528641e74f99fe2f9fd8f2.1585304087.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH luajit 1/2] jit: abort trace recording and execution for C API List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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 "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