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

[Tarantool-patches] [PATCH 0/4] 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  

Oleg Babin (4):
  fiber: rename ref to fiber_ref
  fiber: pass struct fiber into lbox_pushfiber instead of id
  fiber: keep reference to userdata if fiber created once
  fiber: allocate on_stop triggers using mempool

 src/lib/core/fiber.h |   7 ++-
 src/lua/fiber.c      | 134 ++++++++++++++++++-------------------------
 2 files changed, 62 insertions(+), 79 deletions(-)

-- 
2.32.0

[Tarantool-patches] [PATCH 1/4] fiber: rename ref to fiber_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_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

Re: [Tarantool-patches] [PATCH 1/4] fiber: rename ref to fiber_ref

[Igor Munkin via Tarantool-patches]

Oleg,

Thanks for the patch! LGTM since the changes are trivial, but consider
the nits below.

On 11.08.21, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> Ref is a lua reference to fiber storage. This patch just renames
> storage.lua.ref to storage_ref to underline that it's not a

Minor: This line seems ambiguous (at least to me). You're writing about
storage.lua.ref, so the renamed field should be storage.lua.storage_ref.
Feel free to ignore.

> 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;

Typo: You're writing about <fiber_ref> in the commit subject, but the
patch is about <storage_ref>.

>  		} lua;
>  		/**
>  		 * Iproto sync.

<snipped>

> -- 
> 2.32.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 1/4] fiber: rename ref to fiber_ref

[Бабин Олег via Tarantool-patches]

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


Thanks for your review. New commit message:
 
    fiber: rename ref to storage_ref
    
     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. 
>Четверг, 2 сентября 2021, 18:47 +03:00 от Igor Munkin <imun@tarantool.org>:
> 
>Oleg,
>
>Thanks for the patch! LGTM since the changes are trivial, but consider
>the nits below.
>
>On 11.08.21,  olegrok@tarantool.org wrote:
>> From: Oleg Babin < babinoleg@mail.ru >
>>
>> Ref is a lua reference to fiber storage. This patch just renames
>> storage.lua.ref to storage_ref to underline that it's not a
>
>Minor: This line seems ambiguous (at least to me). You're writing about
>storage.lua.ref, so the renamed field should be storage.lua.storage_ref.
>Feel free to ignore.
>
>> 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;
>
>Typo: You're writing about <fiber_ref> in the commit subject, but the
>patch is about <storage_ref>.
>
>> } lua;
>> /**
>> * Iproto sync.
>
><snipped>
>
>> --
>> 2.32.0
>>
>
>--
>Best regards,
>IM 
 
 
--
Oleg Babin
 

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

[Tarantool-patches] [PATCH 2/4] 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

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

[Igor Munkin via Tarantool-patches]

Oleg,

Thanks for the patch! LGTM since the changes are trivial.

-- 
Best regards,
IM

[Tarantool-patches] [PATCH 3/4] 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.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;
 			/**
 			 * 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";
 
 /**
- * @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;
+	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));
+		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 *
@@ -669,28 +657,6 @@ 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)
 {
@@ -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);
-		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 3/4] fiber: keep reference to userdata if fiber created once

[Vladislav Shpilevoy via 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;
> 

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

[Oleg Babin via Tarantool-patches]

Thanks for your review. My answers below.

On 25.08.2021 23:33, Vladislav Shpilevoy wrote:
> 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.

Fixed

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

I'll think about it. I guess it's possible but it's required more time 
and could be done

on the top of this patch.


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

Oh, I've missed it. Malloc is removed.


>> -		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;
>>

Diff:

diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 268ddf9cc..83a6b4850 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -99,7 +99,7 @@ static const char *fiberlib_name = "fiber";
  static int
  lbox_fiber_on_stop(struct trigger *trigger, void *event)
  {
-    struct fiber *f = (struct fiber *) 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);
@@ -122,12 +122,12 @@ lbox_pushfiber(struct lua_State *L, struct fiber *f)
  {
      int ref = f->storage.lua.ref;
      if (ref <= 0) {
-        struct trigger *t = (struct trigger *)malloc(sizeof(*t));
+        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_create(t, lbox_fiber_on_stop, NULL, (trigger_f0)free);
          trigger_add(&f->on_stop, t);

          uint64_t fid = f->fid;
@@ -663,12 +663,6 @@ 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);
-        }
          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 3/4] fiber: keep reference to userdata if fiber created once

[Igor Munkin via Tarantool-patches]

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.

>  			/**
>  			 * 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.

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

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

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

[Бабин Олег via Tarantool-patches]

[-- 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 --]

Re: [Tarantool-patches] [PATCH 3/4] 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>

Please, don't. Libcore should not depend on Lua anyhow.

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

[Oleg Babin via Tarantool-patches]

On 03.09.2021 00:39, Vladislav Shpilevoy wrote:
>> 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>
> Please, don't. Libcore should not depend on Lua anyhow.


I understand you but I don't see another way to initialize refs with 
LUA_NOREF.

I tried to do it only for Lua-created fiber but there is fibers from the 
pool and

they also required to be initialized. And in general any non-lua fiber 
could be find with

fiber.find and referenced in Lua.


Do you have any ideas how it could be fixed?

[Tarantool-patches] [PATCH 4/4] fiber: allocate on_stop triggers using mempool

[Oleg Babin via Tarantool-patches]

From: Oleg Babin <babinoleg@mail.ru>

Previously we use malloc for such purpose however we created
on_stop triggers only if fiber storage was touched. Currently we
create on_stop trigger for each lua fiber. So it seems reasonable
to use mempool for such frequent allocations/deallocations.
---
 src/lua/fiber.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 268ddf9cc..f7c0ca63f 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -36,10 +36,19 @@
 #include "backtrace.h"
 #include "tt_static.h"
 
+#include <small/mempool.h>
 #include <lua.h>
 #include <lauxlib.h>
 #include <lualib.h>
 
+static struct mempool on_stop_triggers_pool;
+
+static void
+on_stop_trigger_free(struct trigger *t)
+{
+	mempool_free(&on_stop_triggers_pool, t);
+}
+
 void
 luaL_testcancel(struct lua_State *L)
 {
@@ -110,7 +119,7 @@ lbox_fiber_on_stop(struct trigger *trigger, void *event)
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, ref);
 	f->storage.lua.ref = LUA_NOREF;
 	trigger_clear(trigger);
-	free(trigger);
+	on_stop_trigger_free(trigger);
 	return 0;
 }
 
@@ -122,12 +131,13 @@ lbox_pushfiber(struct lua_State *L, struct fiber *f)
 {
 	int ref = f->storage.lua.ref;
 	if (ref <= 0) {
-		struct trigger *t = (struct trigger *)malloc(sizeof(*t));
+		struct trigger *t = mempool_alloc(&on_stop_triggers_pool);;
 		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_create(t, lbox_fiber_on_stop, NULL,
+				 on_stop_trigger_free);
 		trigger_add(&f->on_stop, t);
 
 		uint64_t fid = f->fid;
@@ -921,6 +931,9 @@ static const struct luaL_Reg fiberlib[] = {
 void
 tarantool_lua_fiber_init(struct lua_State *L)
 {
+	mempool_create(&on_stop_triggers_pool, &cord()->slabc,
+				   sizeof(struct trigger));
+
 	luaL_register_module(L, fiberlib_name, fiberlib);
 	lua_pop(L, 1);
 	luaL_register_type(L, fiberlib_name, lbox_fiber_meta);
-- 
2.32.0

Re: [Tarantool-patches] [PATCH 4/4] fiber: allocate on_stop triggers using mempool

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

See 4 comments below.

On 11.08.2021 22:33, Oleg Babin via Tarantool-patches wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> Previously we use malloc for such purpose however we created
> on_stop triggers only if fiber storage was touched. Currently we
> create on_stop trigger for each lua fiber. So it seems reasonable
> to use mempool for such frequent allocations/deallocations.

1. Does it give any perf win? This commit specifically. Seems like an
overkill. A trigger allocation might happen only once per fiber's
lifetime, and the fiber's creation is supposed to be much longer than
the trigger's malloc. The commit probably gives a tiny fraction of
percent improvement or does not change anything. Do you have results?

> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
> index 268ddf9cc..f7c0ca63f 100644
> --- a/src/lua/fiber.c
> +++ b/src/lua/fiber.c
> @@ -36,10 +36,19 @@
>  #include "backtrace.h"
>  #include "tt_static.h"
>  
> +#include <small/mempool.h>

2. Please, use "" for non-system headers.

>  #include <lua.h>
>  #include <lauxlib.h>
>  #include <lualib.h>
>  
> @@ -122,12 +131,13 @@ lbox_pushfiber(struct lua_State *L, struct fiber *f)
>  {
>  	int ref = f->storage.lua.ref;
>  	if (ref <= 0) {
> -		struct trigger *t = (struct trigger *)malloc(sizeof(*t));
> +		struct trigger *t = mempool_alloc(&on_stop_triggers_pool);;
>  		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_create(t, lbox_fiber_on_stop, NULL,
> +				 on_stop_trigger_free);

3. Misalignment.

>  		trigger_add(&f->on_stop, t);
>  
>  		uint64_t fid = f->fid;
> @@ -921,6 +931,9 @@ static const struct luaL_Reg fiberlib[] = {
>  void
>  tarantool_lua_fiber_init(struct lua_State *L)
>  {
> +	mempool_create(&on_stop_triggers_pool, &cord()->slabc,
> +				   sizeof(struct trigger));

4. Ditto.

> +
>  	luaL_register_module(L, fiberlib_name, fiberlib);
>  	lua_pop(L, 1);
>  	luaL_register_type(L, fiberlib_name, lbox_fiber_meta);
> 

Re: [Tarantool-patches] [PATCH 4/4] fiber: allocate on_stop triggers using mempool

[Oleg Babin via Tarantool-patches]

Thanks for your review. See my answers below.

On 25.08.2021 23:34, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 4 comments below.
>
> On 11.08.2021 22:33, Oleg Babin via Tarantool-patches wrote:
>> From: Oleg Babin<babinoleg@mail.ru>
>>
>> Previously we use malloc for such purpose however we created
>> on_stop triggers only if fiber storage was touched. Currently we
>> create on_stop trigger for each lua fiber. So it seems reasonable
>> to use mempool for such frequent allocations/deallocations.
> 1. Does it give any perf win? This commit specifically. Seems like an
> overkill. A trigger allocation might happen only once per fiber's
> lifetime, and the fiber's creation is supposed to be much longer than
> the trigger's malloc. The commit probably gives a tiny fraction of
> percent improvement or does not change anything. Do you have results?

It's a good question. I thought thank mempool should be faster than malloc.

We can easily drop this commit. I had some doubts about it.


Benchmark is following:

```

for _ = 1, 10 do
     local start = clock.time()
     for _ = 1, 1e5 do
         fiber.new(function() end)
     end
     fiber.yield()
     print('fiber.new()', clock.time() - start)
end

```

Current result on my machine:

```

fiber.new()    1.7790501117706
fiber.new()    0.23458814620972
fiber.new()    0.2239830493927
fiber.new()    0.25585198402405
fiber.new()    0.26314878463745
fiber.new()    0.17044496536255
fiber.new()    0.23963904380798
fiber.new()    0.2929859161377
fiber.new()    0.16769504547119
fiber.new()    0.26060795783997

```

Previous commit:

```

fiber.new()    1.7500579357147
fiber.new()    0.24660110473633
fiber.new()    0.23380303382874
fiber.new()    0.28128218650818
fiber.new()    0.25019407272339
fiber.new()    0.23627710342407
fiber.new()    0.29107189178467
fiber.new()    0.19264888763428
fiber.new()    0.27617883682251
fiber.new()    0.25651788711548

```


master:

```

fiber.new()    1.6132490634918
fiber.new()    0.23563098907471
fiber.new()    0.23894000053406
fiber.new()    0.25919413566589
fiber.new()    0.29895401000977
fiber.new()    0.24670886993408
fiber.new()    0.26245188713074
fiber.new()    0.21903514862061
fiber.new()    0.19983816146851
fiber.new()    0.23553991317749

```


A bit modified test with fiber.new -> fiber.create

olegrok/fiber-ref HEAD:

```

fiber.create()    0.14023184776306
fiber.create()    0.13392496109009
fiber.create()    0.13862681388855
fiber.create()    0.13287806510925
fiber.create()    0.14014101028442
fiber.create()    0.15336418151855
fiber.create()    0.13403511047363
fiber.create()    0.12675404548645
fiber.create()    0.13705611228943
fiber.create()    0.13896298408508

```

olegrok/fiber-ref HEAD~1:

```

fiber.create()    0.13091707229614
fiber.create()    0.12622594833374
fiber.create()    0.13849806785583
fiber.create()    0.13334107398987
fiber.create()    0.12660312652588
fiber.create()    0.13159799575806
fiber.create()    0.12833094596863
fiber.create()    0.1359851360321
fiber.create()    0.1327109336853
fiber.create()    0.12926387786865

```


master:

```

fiber.create()    0.17535996437073
fiber.create()    0.15730094909668
fiber.create()    0.15560913085938
fiber.create()    0.15467691421509
fiber.create()    0.15472817420959
fiber.create()    0.17342209815979
fiber.create()    0.15398406982422
fiber.create()    0.16387009620667
fiber.create()    0.16766691207886
fiber.create()    0.17136907577515

```



>> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
>> index 268ddf9cc..f7c0ca63f 100644
>> --- a/src/lua/fiber.c
>> +++ b/src/lua/fiber.c
>> @@ -36,10 +36,19 @@
>>   #include "backtrace.h"
>>   #include "tt_static.h"
>>   
>> +#include <small/mempool.h>
> 2. Please, use "" for non-system headers.


Fixed

>>   #include <lua.h>
>>   #include <lauxlib.h>
>>   #include <lualib.h>
>>   
>> @@ -122,12 +131,13 @@ lbox_pushfiber(struct lua_State *L, struct fiber *f)
>>   {
>>   	int ref = f->storage.lua.ref;
>>   	if (ref <= 0) {
>> -		struct trigger *t = (struct trigger *)malloc(sizeof(*t));
>> +		struct trigger *t = mempool_alloc(&on_stop_triggers_pool);;
>>   		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_create(t, lbox_fiber_on_stop, NULL,
>> +				 on_stop_trigger_free);
> 3. Misalignment.

Fixed


>>   		trigger_add(&f->on_stop, t);
>>   
>>   		uint64_t fid = f->fid;
>> @@ -921,6 +931,9 @@ static const struct luaL_Reg fiberlib[] = {
>>   void
>>   tarantool_lua_fiber_init(struct lua_State *L)
>>   {
>> +	mempool_create(&on_stop_triggers_pool, &cord()->slabc,
>> +				   sizeof(struct trigger));
> 4. Ditto.


Fixed

>> +
>>   	luaL_register_module(L, fiberlib_name, fiberlib);
>>   	lua_pop(L, 1);
>>   	luaL_register_type(L, fiberlib_name, lbox_fiber_meta);
>>

Re: [Tarantool-patches] [PATCH 4/4] fiber: allocate on_stop triggers using mempool

[Vladislav Shpilevoy via Tarantool-patches]

On 26.08.2021 08:14, Oleg Babin via Tarantool-patches wrote:
> Thanks for your review. See my answers below.
> 
> On 25.08.2021 23:34, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> See 4 comments below.
>>
>> On 11.08.2021 22:33, Oleg Babin via Tarantool-patches wrote:
>>> From: Oleg Babin<babinoleg@mail.ru>
>>>
>>> Previously we use malloc for such purpose however we created
>>> on_stop triggers only if fiber storage was touched. Currently we
>>> create on_stop trigger for each lua fiber. So it seems reasonable
>>> to use mempool for such frequent allocations/deallocations.
>> 1. Does it give any perf win? This commit specifically. Seems like an
>> overkill. A trigger allocation might happen only once per fiber's
>> lifetime, and the fiber's creation is supposed to be much longer than
>> the trigger's malloc. The commit probably gives a tiny fraction of
>> percent improvement or does not change anything. Do you have results?
> 
> It's a good question. I thought thank mempool should be faster than malloc.
> 
> We can easily drop this commit. I had some doubts about it.

According to the benchmarks it seems the mempool here is slower.
I suggest to drop this commit.

Re: [Tarantool-patches] [PATCH 4/4] fiber: allocate on_stop triggers using mempool

[Oleg Babin via Tarantool-patches]

On 26.08.2021 23:01, Vladislav Shpilevoy wrote:
> On 26.08.2021 08:14, Oleg Babin via Tarantool-patches wrote:
>> Thanks for your review. See my answers below.
>>
>> On 25.08.2021 23:34, Vladislav Shpilevoy wrote:
>>> Thanks for the patch!
>>>
>>> See 4 comments below.
>>>
>>> On 11.08.2021 22:33, Oleg Babin via Tarantool-patches wrote:
>>>> From: Oleg Babin<babinoleg@mail.ru>
>>>>
>>>> Previously we use malloc for such purpose however we created
>>>> on_stop triggers only if fiber storage was touched. Currently we
>>>> create on_stop trigger for each lua fiber. So it seems reasonable
>>>> to use mempool for such frequent allocations/deallocations.
>>> 1. Does it give any perf win? This commit specifically. Seems like an
>>> overkill. A trigger allocation might happen only once per fiber's
>>> lifetime, and the fiber's creation is supposed to be much longer than
>>> the trigger's malloc. The commit probably gives a tiny fraction of
>>> percent improvement or does not change anything. Do you have results?
>> It's a good question. I thought thank mempool should be faster than malloc.
>>
>> We can easily drop this commit. I had some doubts about it.
> According to the benchmarks it seems the mempool here is slower.
> I suggest to drop this commit.


No problem. I removed commit from the branch 
(https://github.com/tarantool/tarantool/commits/olegrok/fiber-ref)

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

[Igor Munkin via Tarantool-patches]

Oleg,

Thanks for the series!

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

Does this PR supersedes another one[1] from you? If it does, please
close #6215 then.

> 
> Oleg Babin (4):
>   fiber: rename ref to fiber_ref
>   fiber: pass struct fiber into lbox_pushfiber instead of id
>   fiber: keep reference to userdata if fiber created once
>   fiber: allocate on_stop triggers using mempool
> 
>  src/lib/core/fiber.h |   7 ++-
>  src/lua/fiber.c      | 134 ++++++++++++++++++-------------------------
>  2 files changed, 62 insertions(+), 79 deletions(-)
> 
> -- 
> 2.32.0
> 

[1]: https://github.com/tarantool/tarantool/pull/6215

-- 
Best regards,
IM

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

[Бабин Олег via Tarantool-patches]

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


Thanks for your review. See my answer below.

  
>Четверг, 2 сентября 2021, 18:48 +03:00 от Igor Munkin <imun@tarantool.org>:
> 
>Oleg,
>
>Thanks for the series!
>
>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.
>>
>> 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
>
>Does this PR supersedes another one[1] from you? If it does, please
>close #6215 then.
 
Yes, let’s close them. Such approach solves my particular task but there are
opinions that we shouldn’t add new functions to current fiber API.
 
>>
>> Oleg Babin (4):
>> fiber: rename ref to fiber_ref
>> fiber: pass struct fiber into lbox_pushfiber instead of id
>> fiber: keep reference to userdata if fiber created once
>> fiber: allocate on_stop triggers using mempool
>>
>> src/lib/core/fiber.h | 7 ++-
>> src/lua/fiber.c | 134 ++++++++++++++++++-------------------------
>> 2 files changed, 62 insertions(+), 79 deletions(-)
>>
>> --
>> 2.32.0
>>
>
>[1]:  https://github.com/tarantool/tarantool/pull/6215
>
>--
>Best regards,
>IM 
 
 
--
Oleg Babin
 

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