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

[Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for your review. See my answers below.

On 03.09.2021 15:23, Igor Munkin wrote:
> Oleg,
>
> Thanks for the fixes! I still have some comments regarding the new
> version of this patch.
>
> On 02.09.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.c |   5 ++
>>   src/lib/core/fiber.h |   5 ++
>>   src/lua/fiber.c      | 113 +++++++++++++------------------------------
>>   3 files changed, 44 insertions(+), 79 deletions(-)
>>
>> 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>
> Totally agree with Vlad here. I guess you have two options:
> * Use the approach similar to <cord_on_yield>.
> * Introduce a constant in src/lib/core/fiber.h and add a static assert
>    in src/lua/utils.h whether this value is the same as in LuaJIT.
>
> Maybe there are other options Vlad can suggest.


I've found that there is lua_nil_ref value in our code. For my purpose 
I've added lua_no_ref.

Incremental diff will be below. Also I will send new series since this 
change was done in separate patch.


>
>>   #include <errno.h>
>>   #include <stdio.h>
>>   #include <stdlib.h>
> <snipped>
>
>> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
>> index 5575f2079..ab0e895eb 100644
>> --- a/src/lua/fiber.c > +++ b/src/lua/fiber.c
>> @@ -87,27 +87,28 @@ luaL_testcancel(struct lua_State *L)
> <snipped>
>
>> +	struct fiber *f = event;
>> +	int storage_ref = f->storage.lua.storage_ref;
> Minor: Why do you need both <storage_ref> and <ref> variables? After
> your change they can be freely dropped.

Removed


>
>> +	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.fid_ref = LUA_NOREF;
>> +	trigger_clear(trigger);
>> +	free(trigger);
>> +	return 0;
>>   }
>>   
>>   /**
>> @@ -116,42 +117,26 @@ lbox_create_weak_table(struct lua_State *L, const char *name)
> <snipped>
>
>> +	int ref = f->storage.lua.fid_ref;
> Minor: It would be nice also to rename <ref> to <fid_ref>.

Fixed.


>> +	if (ref == LUA_NOREF) {
> <snipped>
>
>> -- 
>> 2.32.0
>>


diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 39b67f940..1e5b2fb80 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -32,7 +32,6 @@

  #include <trivia/config.h>
  #include <trivia/util.h>
-#include <lauxlib.h>
  #include <errno.h>
  #include <stdio.h>
  #include <stdlib.h>
@@ -45,6 +44,7 @@
  #include "errinj.h"

  extern void cord_on_yield(void);
+extern int luaL_no_ref;

  #if ENABLE_FIBER_TOP
  #include <x86intrin.h> /* __rdtscp() */
@@ -896,8 +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;
+    fiber->storage.lua.storage_ref = luaL_no_ref;
+    fiber->storage.lua.fid_ref = luaL_no_ref;
      unregister_fid(fiber);
      fiber->fid = 0;
      region_free(&fiber->gc);
@@ -1239,8 +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;
+        fiber->storage.lua.storage_ref = luaL_no_ref;
+        fiber->storage.lua.fid_ref = luaL_no_ref;

          if (fiber_stack_create(fiber, &cord()->slabc,
                         fiber_attr->stack_size)) {
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index f82f4032f..70addd8f6 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -100,11 +100,9 @@ static int
  lbox_fiber_on_stop(struct trigger *trigger, void *event)
  {
      struct fiber *f = event;
-    int storage_ref = f->storage.lua.storage_ref;
-    luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref);
+    luaL_unref(tarantool_L, LUA_REGISTRYINDEX, f->storage.lua.storage_ref);
      f->storage.lua.storage_ref = LUA_NOREF;
-    int ref = f->storage.lua.fid_ref;
-    luaL_unref(tarantool_L, LUA_REGISTRYINDEX, ref);
+    luaL_unref(tarantool_L, LUA_REGISTRYINDEX, f->storage.lua.fid_ref);
      f->storage.lua.fid_ref = LUA_NOREF;
      trigger_clear(trigger);
      free(trigger);
@@ -117,8 +115,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.fid_ref;
-    if (ref == LUA_NOREF) {
+    int fid_ref = f->storage.lua.fid_ref;
+    if (fid_ref == LUA_NOREF) {
          struct trigger *t = malloc(sizeof(*t));
          if (t == NULL) {
              diag_set(OutOfMemory, sizeof(*t), "malloc", "t");
@@ -133,10 +131,10 @@ lbox_pushfiber(struct lua_State *L, struct fiber *f)
          *ptr = fid;
          luaL_getmetatable(L, fiberlib_name);
          lua_setmetatable(L, -2);
-        ref = luaL_ref(L, LUA_REGISTRYINDEX);
-        f->storage.lua.fid_ref = ref;
+        fid_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+        f->storage.lua.fid_ref = fid_ref;
      }
-    lua_rawgeti(L, LUA_REGISTRYINDEX, ref);
+    lua_rawgeti(L, LUA_REGISTRYINDEX, fid_ref);
  }

  static struct fiber *
diff --git a/src/lua/utils.c b/src/lua/utils.c
index c71cd4857..c252d7cdf 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -40,6 +40,7 @@
  #include "uuid/tt_uuid.h"

  int luaL_nil_ref = LUA_REFNIL;
+int luaL_no_ref = LUA_NOREF;

  static int luaT_newthread_ref = LUA_NOREF;

diff --git a/test/unit/core_test_utils.c b/test/unit/core_test_utils.c
index 23452bbfd..9b1789e75 100644
--- a/test/unit/core_test_utils.c
+++ b/test/unit/core_test_utils.c
@@ -35,3 +35,5 @@
  void cord_on_yield(void)
  {
  }
+
+int luaL_no_ref = -2;