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 v4: - Change approach for ref initialization in core/fiber module Changes in v3: - Review fixes by Igor - New patch to make public LUA_NOREF value Changes in v2: - Review fixes by Vlad and Igor - The patch patch that introduced mempool usage is discarded Oleg Babin (4): fiber: rename ref to storage_ref fiber: pass struct fiber into lbox_pushfiber instead of id fiber: correctly initialize storage_ref value fiber: keep reference to userdata if fiber created once src/lib/core/fiber.c | 4 ++ src/lib/core/fiber.h | 9 +++- src/lua/fiber.c | 126 ++++++++++++++----------------------------- 3 files changed, 52 insertions(+), 87 deletions(-) -- 2.32.0
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
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
From: Oleg Babin <babinoleg@mail.ru> Before this patch we used an assumption in lua fiber code that all valid lua refs are positive numbers. However there are special constants defined in lua header for such purpose LUA_NOREF and LUA_REFNIL [1]. This patch introduces special value in fiber module that is equal to LUA_REFNIL to be sure that this value will be correct in future static assert is added. Also this patch introduces several usages of this value - let's make fiber code more clearer and start to initialize ref values with LUA_NOREF value. Also it will be needed for future changes. [1] https://pgl.yoyo.org/luai/i/luaL_ref --- src/lib/core/fiber.c | 2 ++ src/lib/core/fiber.h | 2 ++ src/lua/fiber.c | 7 +++---- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c index 588b78504..b7699372a 100644 --- a/src/lib/core/fiber.c +++ b/src/lib/core/fiber.c @@ -895,6 +895,7 @@ fiber_recycle(struct fiber *fiber) fiber->f = NULL; fiber->wait_pad = NULL; memset(&fiber->storage, 0, sizeof(fiber->storage)); + fiber->storage.lua.storage_ref = FIBER_LUA_NOREF; unregister_fid(fiber); fiber->fid = 0; region_free(&fiber->gc); @@ -1236,6 +1237,7 @@ 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 = FIBER_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..9584b1611 100644 --- a/src/lib/core/fiber.h +++ b/src/lib/core/fiber.h @@ -45,6 +45,8 @@ #include <coro/coro.h> +#define FIBER_LUA_NOREF (-2) + /* * Fiber top doesn't work on ARM processors at the moment, * because we haven't chosen an alternative to rdtsc. diff --git a/src/lua/fiber.c b/src/lua/fiber.c index 5575f2079..96aa73b19 100644 --- a/src/lua/fiber.c +++ b/src/lua/fiber.c @@ -38,7 +38,7 @@ #include <lua.h> #include <lauxlib.h> -#include <lualib.h> +static_assert(FIBER_LUA_NOREF == LUA_NOREF, "FIBER_LUA_NOREF is ok"); void luaL_testcancel(struct lua_State *L) @@ -683,9 +683,8 @@ 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; + f->storage.lua.storage_ref = FIBER_LUA_NOREF; trigger_clear(trigger); free(trigger); return 0; @@ -696,7 +695,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 == FIBER_LUA_NOREF) { struct trigger *t = (struct trigger *) malloc(sizeof(*t)); if (t == NULL) { -- 2.32.0
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 | 2 + src/lib/core/fiber.h | 5 ++ src/lua/fiber.c | 110 +++++++++++++------------------------------ 3 files changed, 39 insertions(+), 78 deletions(-) diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c index b7699372a..3e19f131b 100644 --- a/src/lib/core/fiber.c +++ b/src/lib/core/fiber.c @@ -896,6 +896,7 @@ fiber_recycle(struct fiber *fiber) fiber->wait_pad = NULL; memset(&fiber->storage, 0, sizeof(fiber->storage)); fiber->storage.lua.storage_ref = FIBER_LUA_NOREF; + fiber->storage.lua.fid_ref = FIBER_LUA_NOREF; unregister_fid(fiber); fiber->fid = 0; region_free(&fiber->gc); @@ -1238,6 +1239,7 @@ fiber_new_ex(const char *name, const struct fiber_attr *fiber_attr, } memset(fiber, 0, sizeof(struct fiber)); fiber->storage.lua.storage_ref = FIBER_LUA_NOREF; + fiber->storage.lua.fid_ref = FIBER_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 9584b1611..9ac0cbb9f 100644 --- a/src/lib/core/fiber.h +++ b/src/lib/core/fiber.h @@ -625,6 +625,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 96aa73b19..12836da69 100644 --- a/src/lua/fiber.c +++ b/src/lua/fiber.c @@ -87,27 +87,26 @@ 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; + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, f->storage.lua.storage_ref); + f->storage.lua.storage_ref = FIBER_LUA_NOREF; + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, f->storage.lua.fid_ref); + f->storage.lua.fid_ref = FIBER_LUA_NOREF; + trigger_clear(trigger); + free(trigger); + return 0; } /** @@ -116,42 +115,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 fid_ref = f->storage.lua.fid_ref; + if (fid_ref == FIBER_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); + fid_ref = luaL_ref(L, LUA_REGISTRYINDEX); + f->storage.lua.fid_ref = fid_ref; } + lua_rawgeti(L, LUA_REGISTRYINDEX, fid_ref); } static struct fiber * @@ -669,41 +652,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; - luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref); - f->storage.lua.storage_ref = FIBER_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 == FIBER_LUA_NOREF) { - 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
Hi! Thanks for the fixes! LGTM.
On Wed, Sep 08, 2021 at 09:19:58PM +0300, Oleg Babin via Tarantool-patches wrote:
> [2] https://github.com/tarantool/tarantool/pull/6280
Reviewed in the PR.
Thanks for your review. On 09.09.2021 13:21, Vladimir Davydov via Tarantool-patches wrote: > On Wed, Sep 08, 2021 at 09:19:58PM +0300, Oleg Babin via Tarantool-patches wrote: >> [2] https://github.com/tarantool/tarantool/pull/6280 > Reviewed in the PR. Fixed. Diff: diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h index 9ac0cbb9f..67e1a55e3 100644 --- a/src/lib/core/fiber.h +++ b/src/lib/core/fiber.h @@ -45,6 +45,14 @@ #include <coro/coro.h> +/* + * This constant is the same as LUA_NOREF. It should be used + * to initialize all Lua references in struct fiber. + * Since this module is independent on Lua the constant is + * defined directly. Value should be checked with + * static_assert in appropriate places to catch possible + * LUA_NOREF value changes in future. + */ #define FIBER_LUA_NOREF (-2) /* @@ -627,7 +635,7 @@ struct fiber { struct lua_State *stack; /** * Optional reference to userdata - * represented current fiber id in Lua. + * representing current fiber id in Lua. */ int fid_ref; /** diff --git a/src/lua/fiber.c b/src/lua/fiber.c index 12836da69..95634a4d0 100644 --- a/src/lua/fiber.c +++ b/src/lua/fiber.c @@ -38,6 +38,7 @@ #include <lua.h> #include <lauxlib.h> + static_assert(FIBER_LUA_NOREF == LUA_NOREF, "FIBER_LUA_NOREF is ok"); void