From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 453F625C5C for ; Thu, 7 Jun 2018 13:02:38 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id DcL-MXnrbp19 for ; Thu, 7 Jun 2018 13:02:38 -0400 (EDT) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id ED84325C98 for ; Thu, 7 Jun 2018 13:02:37 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3 3/4] session: introduce binary box.session.push References: <6952c89275280b57a7d94b759234488b1a1cff20.1527886471.git.v.shpilevoy@tarantool.org> <20180607125352.GA30262@chai> From: Vladislav Shpilevoy Message-ID: <3c142fdd-fb7b-bd60-4615-f9152fd8b8c9@tarantool.org> Date: Thu, 7 Jun 2018 20:02:35 +0300 MIME-Version: 1.0 In-Reply-To: <20180607125352.GA30262@chai> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Konstantin Osipov On 07/06/2018 15:53, Konstantin Osipov wrote: > Hi, > > My code review for this patch is on branch > box-session-push-kostja. > > What still needs to be done: > > - convert push.test to box-tap I have removed assertions and left the test be output checking. Push test does many table members checking, and it is simpler to just output the tables. Assertions were in a single place were a table contains 400 members. But ok, now I did it like in vinyl/select_consistency test. > - think about alternatives which would preserve sync in a Lua > routine. Looks like using an upvalue would do > https://www.lua.org/pil/27.3.3.html I had investigated it before you have recommended and before I started the patch. But ok, I have investigated it again, and wrote the simple patch, that shows upvalues to be unusable as sync storage. See the diff below and the explanation under the diff. +++ b/src/box/lua/call.c @@ -434,8 +434,9 @@ box_process_lua(struct call_request *request, struct port *base, - lua_pushcfunction(L, handler); + uint64_t sync = request->header->sync; + lua_pushinteger(L, sync); + lua_pushcclosure(L, handler, 1); +++ b/src/box/lua/session.c @@ -370,6 +370,9 @@ lbox_session_push(struct lua_State *L) + uint64_t sync = lua_tointeger(L, lua_upvalueindex(1)); + uint64_t f_sync = fiber_sync(fiber()); + say_info("%llu, %llu", (unsigned long long) sync, f_sync); if (session_push(current_session(), &port) != 0) { After running push.test.lua I found say_info() printing always 0 + real sync. So the upvalue is actually nil here. C upvalues are not the same as Lua upvalues. C upvalue can be associated with a single function only, while the Lua ones can be referenced in multiple and enclosed functions. C upvalue can be accessed only from the function owning it. In the diff above I have pushed sync as an upvalue and tried to get it in lbox_session_push function. But the upvalue was not available. Lets look at the implementation of lua_tointeger(L, lua_upvalueindex(1)). lua_upvalueindex(idx): #define LUA_GLOBALSINDEX (-10002) #define lua_upvalueindex(i) (LUA_GLOBALSINDEX-(i)) Nothing is interesting. Upvalue index is just an index. lua_tointeger(L, idx): LUA_API lua_Integer lua_tointeger(lua_State *L, int idx) { cTValue *o = index2adr(L, idx); ... index2addr(L, idx): .... } else { GCfunc *fn = curr_func(L); api_check(L, fn->c.gct == ~LJ_TFUNC && !isluafunc(fn)); if (idx == LUA_ENVIRONINDEX) { TValue *o = &G(L)->tmptv; settabV(L, o, tabref(fn->c.env)); return o; } else { idx = LUA_GLOBALSINDEX - idx; return idx <= fn->c.nupvalues ? &fn->c.upvalue[idx-1] : niltv(L); } } As you see, upvalue array is got from curr_func(L). lbox_session_push != C closure I had pushed earlier. And it has no upvalues. Summary: C upvalues can not be used as a sync storage.