From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id BF79E6EC55; Wed, 25 Aug 2021 23:33:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BF79E6EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629923625; bh=yW+YJ/iyWmHNpaYB5dITVUOXiEqr14aIR2mZVFABCFE=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=b/sNJAPpSPvOtf9aTWDtZ9R9tFG3RP6phOrlbhhszFKNIaYz9ILEl5hhKsQ69+c6e u82KhwtplpyuIFdEjXbwGuFa+Kjy2PSkhFRPvsCsPk7Qn1Ay+QPgmtf5x2yZ5Y8i3O Qf+tA0gUaCp9DxG2qtAYxk8vXLb7omFIggrhXmuc= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [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 2299E6EC55 for ; Wed, 25 Aug 2021 23:33:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2299E6EC55 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1mIzai-00061N-1l; Wed, 25 Aug 2021 23:33:44 +0300 To: olegrok@tarantool.org, imun@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: <3453bc40af586cf4242863f1307f72fe5c5a408a.1628713520.git.babinoleg@mail.ru> Message-ID: Date: Wed, 25 Aug 2021 22:33:42 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <3453bc40af586cf4242863f1307f72fe5c5a408a.1628713520.git.babinoleg@mail.ru> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD91BCCB18F2C129F87F36E61E9E4584E9D182A05F53808504034E3FC91E1D36A9AC84B394235C68ECB7D0081A9CA46BDA78585966BA2981F47 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7C579B1C3ABE6C709C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE772FD67E8B75B52AFEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F25C19A10E69E8D7724D5B501625AB38B1CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F924B32C592EA89F389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC82FFDA4F57982C5F4F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B08F9A42B2210255C75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5A7D1AC590ED73190C2CF5C4D80CCFFB88C380F2F00534C61D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA753177526CD55AFC11410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3458239C6F30CE7ACF2E66852F584D3AC7EC1E441ABE5DF376E971E14ED2D71C3423AEAB0D6D6E28EA1D7E09C32AA3244C56FB442A9204A7ADA8BA104A8C6AE305250262A5EE9971B0FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojXSF/Tsl6M2OimufidZ0COw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D92BA9A3DDC6B90782940CD691E43C2983841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 3/4] fiber: keep reference to userdata if fiber created once X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! See 5 comments below. > diff --git a/src/lua/fiber.c b/src/lua/fiber.c > index 5575f2079..268ddf9cc 100644 > --- a/src/lua/fiber.c > +++ b/src/lua/fiber.c > @@ -87,27 +87,31 @@ luaL_testcancel(struct lua_State *L) > static const char *fiberlib_name = "fiber"; > > /** > - * @pre: stack top contains a table > - * @post: sets table field specified by name of the table on top > - * of the stack to a weak kv table and pops that weak table. > + * Trigger invoked when the fiber has stopped execution of its > + * current request. Only purpose - delete storage.lua.ref and > + * storage.lua.storage_ref keeping a reference of Lua > + * fiber and fiber.storage objects. Unlike Lua stack, > + * Lua fiber storage may be created not only for fibers born from > + * Lua land. For example, an IProto request may execute a Lua > + * function, which can create the storage. Trigger guarantees, > + * that even for non-Lua fibers the Lua storage is destroyed. > */ > -static void > -lbox_create_weak_table(struct lua_State *L, const char *name) > +static int > +lbox_fiber_on_stop(struct trigger *trigger, void *event) > { > - lua_newtable(L); > - /* and a metatable */ > - lua_newtable(L); > - /* weak keys and values */ > - lua_pushstring(L, "kv"); > - /* pops 'kv' */ > - lua_setfield(L, -2, "__mode"); > - /* pops the metatable */ > - lua_setmetatable(L, -2); > - /* assigns and pops table */ > - lua_setfield(L, -2, name); > - /* gets memoize back. */ > - lua_getfield(L, -1, name); > - assert(! lua_isnil(L, -1)); > + struct fiber *f = (struct fiber *) event; 1. In new code we do not use whitespaces after unary operators, including type casts. But here you don't need a cast. void * is implicitly compatible with any other pointer type in C. > + int storage_ref = f->storage.lua.storage_ref; > + if (storage_ref > 0) { > + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref); > + f->storage.lua.storage_ref = LUA_NOREF; > + } > + int ref = f->storage.lua.ref; > + assert(ref > 0); > + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, ref); > + f->storage.lua.ref = LUA_NOREF; > + trigger_clear(trigger); > + free(trigger); > + return 0; > } > > /** > @@ -116,42 +120,26 @@ lbox_create_weak_table(struct lua_State *L, const char *name) > static void > lbox_pushfiber(struct lua_State *L, struct fiber *f) > { > - /* > - * Use 'memoize' pattern and keep a single userdata for > - * the given fiber. This is important to not run __gc > - * twice for a copy of an attached fiber -- __gc should > - * not remove attached fiber's coro prematurely. > - */ > - luaL_getmetatable(L, fiberlib_name); > - lua_getfield(L, -1, "memoize"); > - if (lua_isnil(L, -1)) { > - /* first access - instantiate memoize */ > - /* pop the nil */ > - lua_pop(L, 1); > - /* create memoize table */ > - lbox_create_weak_table(L, "memoize"); > - } > - /* Find out whether the fiber is already in the memoize table. */ > - uint64_t fid = f->fid; > - luaL_pushuint64(L, fid); > - lua_gettable(L, -2); > - if (lua_isnil(L, -1)) { > - /* no userdata for fiber created so far */ > - /* pop the nil */ > - lua_pop(L, 1); > - /* push the key back */ > - luaL_pushuint64(L, fid); > + int ref = f->storage.lua.ref; > + if (ref <= 0) { > + struct trigger *t = (struct trigger *)malloc(sizeof(*t)); 2. No need to cast, in C it works as is. > + if (t == NULL) { > + diag_set(OutOfMemory, sizeof(*t), "malloc", "t"); > + luaT_error(L); > + } > + trigger_create(t, lbox_fiber_on_stop, NULL, (trigger_f0) free); 3. Should not be a whitespace after the cast. > + trigger_add(&f->on_stop, t); > + > + uint64_t fid = f->fid; > /* create a new userdata */ > uint64_t *ptr = lua_newuserdata(L, sizeof(*ptr)); > *ptr = fid; 4. Did you consider pushing the fiber's pointer instead of its ID? And keep the fiber struct from deletion until it has no refs in Lua anymore. That would eliminate lookup in fiber hash on each attempt to access it. Also might make it simpler to return stuff like fiber.csw in Lua. I remember there was a problem with the fiber being deleted by the time you try to access its members. This would solve it. > luaL_getmetatable(L, fiberlib_name); > lua_setmetatable(L, -2); > - /* memoize it */ > - lua_settable(L, -3); > - luaL_pushuint64(L, fid); > - /* get it back */ > - lua_gettable(L, -2); > + ref = luaL_ref(L, LUA_REGISTRYINDEX); > + f->storage.lua.ref = ref; > } > + lua_rawgeti(L, LUA_REGISTRYINDEX, ref); > } > @@ -703,8 +669,6 @@ lbox_fiber_storage(struct lua_State *L) > diag_set(OutOfMemory, sizeof(*t), "malloc", "t"); > return luaT_error(L); > } > - trigger_create(t, lbox_fiber_on_stop, NULL, (trigger_f0) free); 5. The trigger 't' now is not used and leaks. > - trigger_add(&f->on_stop, t); > lua_newtable(L); /* create local storage on demand */ > storage_ref = luaL_ref(L, LUA_REGISTRYINDEX); > f->storage.lua.storage_ref = storage_ref; >