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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.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.

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