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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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