Четверг, 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.

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