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

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

[Oleg Babin via Tarantool-patches]

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.

Initially changes were inspired by #6210 [1] but current patchset
doesn't introduce anything new just improves performance of
existing functions.
Some results are available also in related github PR [2].

  [1] https://github.com/tarantool/tarantool/issues/6210
  [2] https://github.com/tarantool/tarantool/pull/6280

Changes in v2:
  - Review fixes by Vlad and Igor
  - The patch patch that introduced mempool usage is discarded

Oleg Babin (3):
  fiber: rename ref to storage_ref
  fiber: pass struct fiber into lbox_pushfiber instead of id
  fiber: keep reference to userdata if fiber created once

 src/lib/core/fiber.c |   5 ++
 src/lib/core/fiber.h |   7 ++-
 src/lua/fiber.c      | 124 ++++++++++++++-----------------------------
 3 files changed, 51 insertions(+), 85 deletions(-)

-- 
2.32.0

[Tarantool-patches] [PATCH v2 1/3] fiber: rename ref to storage_ref

[Oleg Babin via Tarantool-patches]

From: Oleg Babin <babinoleg@mail.ru>

Ref is a lua reference to fiber storage. This patch just renames
storage.lua.ref to storage.lua.storage_ref to underline that
it's not a reference to fiber itself and needed only for fiber
storage.
---
 src/lib/core/fiber.h |  2 +-
 src/lua/fiber.c      | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index b85625ba7..593847df7 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -626,7 +626,7 @@ struct fiber {
 			/**
 			 * Optional fiber.storage Lua reference.
 			 */
-			int ref;
+			int storage_ref;
 		} lua;
 		/**
 		 * Iproto sync.
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 38394666b..83dcbe2fa 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -670,7 +670,7 @@ lbox_fiber_name(struct lua_State *L)
 
 /**
  * Trigger invoked when the fiber has stopped execution of its
- * current request. Only purpose - delete storage.lua.ref keeping
+ * current request. Only purpose - delete storage.lua.storage_ref keeping
  * a reference of Lua fiber.storage object. 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
@@ -681,10 +681,10 @@ static int
 lbox_fiber_on_stop(struct trigger *trigger, void *event)
 {
 	struct fiber *f = (struct fiber *) event;
-	int storage_ref = f->storage.lua.ref;
+	int storage_ref = f->storage.lua.storage_ref;
 	assert(storage_ref > 0);
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref);
-	f->storage.lua.ref = LUA_NOREF;
+	f->storage.lua.storage_ref = LUA_NOREF;
 	trigger_clear(trigger);
 	free(trigger);
 	return 0;
@@ -694,7 +694,7 @@ static int
 lbox_fiber_storage(struct lua_State *L)
 {
 	struct fiber *f = lbox_checkfiber(L, 1);
-	int storage_ref = f->storage.lua.ref;
+	int storage_ref = f->storage.lua.storage_ref;
 	if (storage_ref <= 0) {
 		struct trigger *t = (struct trigger *)
 			malloc(sizeof(*t));
@@ -706,7 +706,7 @@ lbox_fiber_storage(struct lua_State *L)
 		trigger_add(&f->on_stop, t);
 		lua_newtable(L); /* create local storage on demand */
 		storage_ref = luaL_ref(L, LUA_REGISTRYINDEX);
-		f->storage.lua.ref = storage_ref;
+		f->storage.lua.storage_ref = storage_ref;
 	}
 	lua_rawgeti(L, LUA_REGISTRYINDEX, storage_ref);
 	return 1;
-- 
2.32.0

[Tarantool-patches] [PATCH v2 2/3] fiber: pass struct fiber into lbox_pushfiber instead of id

[Oleg Babin via Tarantool-patches]

From: Oleg Babin <babinoleg@mail.ru>

For future changes we will need the fiber object, not only its id.
---
 src/lua/fiber.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 83dcbe2fa..5575f2079 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -114,7 +114,7 @@ lbox_create_weak_table(struct lua_State *L, const char *name)
  * Push a userdata for the given fiber onto Lua stack.
  */
 static void
-lbox_pushfiber(struct lua_State *L, uint64_t fid)
+lbox_pushfiber(struct lua_State *L, struct fiber *f)
 {
 	/*
 	 * Use 'memoize'  pattern and keep a single userdata for
@@ -132,6 +132,7 @@ lbox_pushfiber(struct lua_State *L, uint64_t fid)
 		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)) {
@@ -501,7 +502,7 @@ fiber_create(struct lua_State *L)
 	/* Move the arguments to the new coro */
 	lua_xmove(L, child_L, lua_gettop(L));
 	/* XXX: 'fiber' is leaked if this throws a Lua error. */
-	lbox_pushfiber(L, f->fid);
+	lbox_pushfiber(L, f);
 	/* Pass coro_ref via lua stack so that we don't have to pass it
 	 * as an argument of fiber_run function.
 	 * No function will work with child_L until the function is called.
@@ -755,7 +756,7 @@ lbox_fiber_yield(struct lua_State *L)
 static int
 lbox_fiber_self(struct lua_State *L)
 {
-	lbox_pushfiber(L, fiber()->fid);
+	lbox_pushfiber(L, fiber());
 	return 1;
 }
 
@@ -767,7 +768,7 @@ lbox_fiber_find(struct lua_State *L)
 	uint64_t fid = luaL_touint64(L, -1);
 	struct fiber *f = fiber_find(fid);
 	if (f)
-		lbox_pushfiber(L, f->fid);
+		lbox_pushfiber(L, f);
 	else
 		lua_pushnil(L);
 	return 1;
-- 
2.32.0

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

[Oleg Babin via Tarantool-patches]

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>
 #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 593847df7..2d4443544 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 id in Lua.
+			 */
+			int fid_ref;
 			/**
 			 * Optional fiber.storage Lua reference.
 			 */
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)
 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.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
+ * 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 = event;
+	int storage_ref = f->storage.lua.storage_ref;
+	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)
 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.fid_ref;
+	if (ref == LUA_NOREF) {
+		struct trigger *t = 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.fid_ref = ref;
 	}
+	lua_rawgeti(L, LUA_REGISTRYINDEX, ref);
 }
 
 static struct fiber *
@@ -669,42 +654,12 @@ lbox_fiber_name(struct lua_State *L)
 	}
 }
 
-/**
- * Trigger invoked when the fiber has stopped execution of its
- * current request. Only purpose - delete storage.lua.storage_ref keeping
- * a reference of Lua fiber.storage object. 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 int
-lbox_fiber_on_stop(struct trigger *trigger, void *event)
-{
-	struct fiber *f = (struct fiber *) event;
-	int storage_ref = f->storage.lua.storage_ref;
-	assert(storage_ref > 0);
-	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref);
-	f->storage.lua.storage_ref = LUA_NOREF;
-	trigger_clear(trigger);
-	free(trigger);
-	return 0;
-}
-
 static int
 lbox_fiber_storage(struct lua_State *L)
 {
 	struct fiber *f = lbox_checkfiber(L, 1);
 	int storage_ref = f->storage.lua.storage_ref;
 	if (storage_ref <= 0) {
-		struct trigger *t = (struct trigger *)
-			malloc(sizeof(*t));
-		if (t == NULL) {
-			diag_set(OutOfMemory, sizeof(*t), "malloc", "t");
-			return luaT_error(L);
-		}
-		trigger_create(t, lbox_fiber_on_stop, NULL, (trigger_f0) free);
-		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;
-- 
2.32.0

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

[Oleg Babin via Tarantool-patches]

Applied and force-pushed small fix:


diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index ab0e895eb..f82f4032f 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -659,7 +659,7 @@ lbox_fiber_storage(struct lua_State *L)
  {
         struct fiber *f = lbox_checkfiber(L, 1);
         int storage_ref = f->storage.lua.storage_ref;
-       if (storage_ref <= 0) {
+       if (storage_ref == LUA_NOREF) {
                 lua_newtable(L); /* create local storage on demand */
                 storage_ref = luaL_ref(L, LUA_REGISTRYINDEX);
                 f->storage.lua.storage_ref = storage_ref;

On 02.09.2021 21:13, Oleg Babin via Tarantool-patches 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>
>   #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 593847df7..2d4443544 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 id in Lua.
> +			 */
> +			int fid_ref;
>   			/**
>   			 * Optional fiber.storage Lua reference.
>   			 */
> 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)
>   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.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
> + * 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 = event;
> +	int storage_ref = f->storage.lua.storage_ref;
> +	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)
>   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.fid_ref;
> +	if (ref == LUA_NOREF) {
> +		struct trigger *t = 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.fid_ref = ref;
>   	}
> +	lua_rawgeti(L, LUA_REGISTRYINDEX, ref);
>   }
>   
>   static struct fiber *
> @@ -669,42 +654,12 @@ lbox_fiber_name(struct lua_State *L)
>   	}
>   }
>   
> -/**
> - * Trigger invoked when the fiber has stopped execution of its
> - * current request. Only purpose - delete storage.lua.storage_ref keeping
> - * a reference of Lua fiber.storage object. 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 int
> -lbox_fiber_on_stop(struct trigger *trigger, void *event)
> -{
> -	struct fiber *f = (struct fiber *) event;
> -	int storage_ref = f->storage.lua.storage_ref;
> -	assert(storage_ref > 0);
> -	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref);
> -	f->storage.lua.storage_ref = LUA_NOREF;
> -	trigger_clear(trigger);
> -	free(trigger);
> -	return 0;
> -}
> -
>   static int
>   lbox_fiber_storage(struct lua_State *L)
>   {
>   	struct fiber *f = lbox_checkfiber(L, 1);
>   	int storage_ref = f->storage.lua.storage_ref;
>   	if (storage_ref <= 0) {
> -		struct trigger *t = (struct trigger *)
> -			malloc(sizeof(*t));
> -		if (t == NULL) {
> -			diag_set(OutOfMemory, sizeof(*t), "malloc", "t");
> -			return luaT_error(L);
> -		}
> -		trigger_create(t, lbox_fiber_on_stop, NULL, (trigger_f0) free);
> -		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;

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

[Igor Munkin via Tarantool-patches]

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.

>  #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.

> +	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>.

> +	if (ref == LUA_NOREF) {

<snipped>

> -- 
> 2.32.0
> 

-- 
Best regards,
IM

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

[Oleg Babin via Tarantool-patches]

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;

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

[Vladislav Shpilevoy via Tarantool-patches]

>> 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 have no better ideas. But among these 2 the one with a constant in
fiber.h and a static assert looks better. Less dependencies.