Re: [Tarantool-patches] [PATCH 3/4] fiber: keep reference to userdata if fiber created once

["Бабин Олег via Tarantool-patches" <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 8613 bytes --]


Thanks for your review. See my answers below.

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

[-- Attachment #2: Type: text/html, Size: 12300 bytes --]