<HTML><BODY><div>Thanks for your review. See my answers below.<br><br> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Четверг, 2 сентября 2021, 18:48 +03:00 от Igor Munkin <imun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16305976901924663803_BODY">Oleg,<br><br>Thanks for the patch! Additionally thank you for purging this memoize<br>hell: now everything is much clearer and the platform performance<br>becomes better. The changes are OK in general, but please consider my<br>comments below.<br><br>On 11.08.21, <a href="/compose?To=olegrok@tarantool.org">olegrok@tarantool.org</a> wrote:<br>> From: Oleg Babin <<a href="/compose?To=babinoleg@mail.ru">babinoleg@mail.ru</a>><br>><br>> This patch reworks approach to fiber management in Lua. Before<br>> this patch each action that should return fiber led to new<br>> userdata creation that was quite slow and made GC suffer. This<br>> patch introduces new field in struct fiber to store a reference to<br>> userdata that was created once for a fiber. It allows speedup<br>> operations as fiber.self() and fiber.id().<br>> Simple benchmark shows that access to fiber storage is faster in<br>> two times, fiber.find() - 2-3 times and fiber.new/create functions<br>> don't have any changes.<br>> ---<br>> src/lib/core/fiber.h | 5 ++<br>> src/lua/fiber.c | 110 +++++++++++++++----------------------------<br>> 2 files changed, 42 insertions(+), 73 deletions(-)<br>><br>> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h<br>> index 593847df7..3e9663a1a 100644<br>> --- a/src/lib/core/fiber.h<br>> +++ b/src/lib/core/fiber.h<br>> @@ -623,6 +623,11 @@ struct fiber {<br>> * Should not be used in other fibers.<br>> */<br>> struct lua_State *stack;<br>> + /**<br>> + * Optional reference to userdata<br>> + * represented current fiber in Lua.<br>> + */<br>> + int ref;<br><br>I see you didn't draw conclusions from your first patch :)<br>I propose to name this field <fid_ref>, so one clearly understand what<br>is stored in that userdata, and no additional patches with rename are<br>necessary in future.</div></div></div></div></blockquote></div><div> </div><div>Good point. Fixed.</div><div> </div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> /**<br>> * Optional fiber.storage Lua reference.<br>> */<br>> diff --git a/src/lua/fiber.c b/src/lua/fiber.c<br>> index 5575f2079..268ddf9cc 100644<br>> --- a/src/lua/fiber.c<br>> +++ b/src/lua/fiber.c<br>> @@ -87,27 +87,31 @@ luaL_testcancel(struct lua_State *L)<br>> static const char *fiberlib_name = "fiber";<br>><br><br><snipped><br><br>> +static int<br>> +lbox_fiber_on_stop(struct trigger *trigger, void *event)<br>> {<br><br><snipped><br><br>> + struct fiber *f = (struct fiber *) event;<br>> + int storage_ref = f->storage.lua.storage_ref;<br>> + if (storage_ref > 0) {<br><br>I don't like this. At first, <luaL_unref> handle this case underneat, so<br>if there is invalid reference or LUA_REFNIL / LUA_NOREF, everything<br>works fine. But the worst part is not the redundancy, but comparing<br><storage_ref> with some magic numbers. Lua provides no guarantees<br>LUA_REFNIL / LUA_NOREF values are not changed between the particular<br>versions. Do not rely on the internal constants outside of LuaJIT<br>sources. Please drop both assertions and this particular <if> condition.</div></div></div></div></blockquote></div><div> </div><div>I agree and I’ll fix it. I think the main issue here that we didn’t</div><div>assign all refs to NOREF and such values was initially filled with memset(0).</div><div>Currently it should be more clear.</div><div> </div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>> + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref);<br>> + f->storage.lua.storage_ref = LUA_NOREF;<br>> + }<br>> + int ref = f->storage.lua.ref;<br>> + assert(ref > 0);<br>> + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, ref);<br>> + f->storage.lua.ref = LUA_NOREF;<br>> + trigger_clear(trigger);<br>> + free(trigger);<br>> + return 0;<br>> }<br>><br>> /**<br>> @@ -116,42 +120,26 @@ lbox_create_weak_table(struct lua_State *L, const char *name)<br>> static void<br>> lbox_pushfiber(struct lua_State *L, struct fiber *f)<br>> {<br><br><snipped><br><br>> + int ref = f->storage.lua.ref;<br>> + if (ref <= 0) {<br><br>Again, do not rely on the internal values. Just compare ref with<br>LUA_NOREF (comparison with LUA_REFNIL is not required, since this is<br>particular value when nil slot is used in <luaL_ref>).</div></div></div></div></blockquote></div><div>Fixed as well. Thanks.</div><div> </div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> + struct trigger *t = (struct trigger *)malloc(sizeof(*t));<br>> + if (t == NULL) {<br>> + diag_set(OutOfMemory, sizeof(*t), "malloc", "t");<br>> + luaT_error(L);<br>> + }<br>> + trigger_create(t, lbox_fiber_on_stop, NULL, (trigger_f0) free);<br>> + trigger_add(&f->on_stop, t);<br>> +<br>> + uint64_t fid = f->fid;<br>> /* create a new userdata */<br>> uint64_t *ptr = lua_newuserdata(L, sizeof(*ptr));<br>> *ptr = fid;<br>> luaL_getmetatable(L, fiberlib_name);<br>> lua_setmetatable(L, -2);<br>> - /* memoize it */<br>> - lua_settable(L, -3);<br>> - luaL_pushuint64(L, fid);<br>> - /* get it back */<br>> - lua_gettable(L, -2);<br>> + ref = luaL_ref(L, LUA_REGISTRYINDEX);<br>> + f->storage.lua.ref = ref;<br>> }<br>> + lua_rawgeti(L, LUA_REGISTRYINDEX, ref);<br>> }<br>><br>> static struct fiber *<br><br><snipped><br><br>> --<br>> 2.32.0<br>><br><br>--<br>Best regards,<br>IM</div></div></div></div></blockquote><div> </div><div>Diff:</div><div>diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c<br>index 588b78504..39b67f940 100644<br>--- a/src/lib/core/fiber.c<br>+++ b/src/lib/core/fiber.c<br>@@ -32,6 +32,7 @@<br> <br> #include <trivia/config.h><br> #include <trivia/util.h><br>+#include <lauxlib.h><br> #include <errno.h><br> #include <stdio.h><br> #include <stdlib.h><br>@@ -895,6 +896,8 @@ fiber_recycle(struct fiber *fiber)<br> fiber->f = NULL;<br> fiber->wait_pad = NULL;<br> memset(&fiber->storage, 0, sizeof(fiber->storage));<br>+ fiber->storage.lua.storage_ref = LUA_NOREF;<br>+ fiber->storage.lua.fid_ref = LUA_NOREF;<br> unregister_fid(fiber);<br> fiber->fid = 0;<br> region_free(&fiber->gc);<br>@@ -1236,6 +1239,8 @@ fiber_new_ex(const char *name, const struct fiber_attr *fiber_attr,<br> return NULL;<br> }<br> memset(fiber, 0, sizeof(struct fiber));<br>+ fiber->storage.lua.storage_ref = LUA_NOREF;<br>+ fiber->storage.lua.fid_ref = LUA_NOREF;<br> <br> if (fiber_stack_create(fiber, &cord()->slabc,<br> fiber_attr->stack_size)) {<br>diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h<br>index 3e9663a1a..2d4443544 100644<br>--- a/src/lib/core/fiber.h<br>+++ b/src/lib/core/fiber.h<br>@@ -625,9 +625,9 @@ struct fiber {<br> struct lua_State *stack;<br> /**<br> * Optional reference to userdata<br>- * represented current fiber in Lua.<br>+ * represented current fiber id in Lua.<br> */<br>- int ref;<br>+ int fid_ref;<br> /**<br> * Optional fiber.storage Lua reference.<br> */<br>diff --git a/src/lua/fiber.c b/src/lua/fiber.c<br>index 83a6b4850..ab0e895eb 100644<br>--- a/src/lua/fiber.c<br>+++ b/src/lua/fiber.c<br>@@ -88,7 +88,7 @@ static const char *fiberlib_name = "fiber";<br> <br> /**<br> * Trigger invoked when the fiber has stopped execution of its<br>- * current request. Only purpose - delete storage.lua.ref and<br>+ * current request. Only purpose - delete storage.lua.fid_ref and<br> * storage.lua.storage_ref keeping a reference of Lua<br> * fiber and fiber.storage objects. Unlike Lua stack,<br> * Lua fiber storage may be created not only for fibers born from<br>@@ -101,14 +101,11 @@ lbox_fiber_on_stop(struct trigger *trigger, void *event)<br> {<br> struct fiber *f = event;<br> int storage_ref = f->storage.lua.storage_ref;<br>- if (storage_ref > 0) {<br>- luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref);<br>- f->storage.lua.storage_ref = LUA_NOREF;<br>- }<br>- int ref = f->storage.lua.ref;<br>- assert(ref > 0);<br>+ luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref);<br>+ f->storage.lua.storage_ref = LUA_NOREF;<br>+ int ref = f->storage.lua.fid_ref;<br> luaL_unref(tarantool_L, LUA_REGISTRYINDEX, ref);<br>- f->storage.lua.ref = LUA_NOREF;<br>+ f->storage.lua.fid_ref = LUA_NOREF;<br> trigger_clear(trigger);<br> free(trigger);<br> return 0;<br>@@ -120,8 +117,8 @@ lbox_fiber_on_stop(struct trigger *trigger, void *event)<br> static void<br> lbox_pushfiber(struct lua_State *L, struct fiber *f)<br> {<br>- int ref = f->storage.lua.ref;<br>- if (ref <= 0) {<br>+ int ref = f->storage.lua.fid_ref;<br>+ if (ref == LUA_NOREF) {<br> struct trigger *t = malloc(sizeof(*t));<br> if (t == NULL) {<br> diag_set(OutOfMemory, sizeof(*t), "malloc", "t");<br>@@ -137,7 +134,7 @@ lbox_pushfiber(struct lua_State *L, struct fiber *f)<br> luaL_getmetatable(L, fiberlib_name);<br> lua_setmetatable(L, -2);<br> ref = luaL_ref(L, LUA_REGISTRYINDEX);<br>- f->storage.lua.ref = ref;<br>+ f->storage.lua.fid_ref = ref;<br> }<br> lua_rawgeti(L, LUA_REGISTRYINDEX, ref);<br> }<br> </div><div> </div><div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Oleg Babin</div></div></div><div> </div></div></BODY></HTML>