[Tarantool-patches] [PATCH 3/4] fiber: keep reference to userdata if fiber created once
Бабин Олег
olegrok at tarantool.org
Thu Sep 2 20:59:33 MSK 2021
Thanks for your review. See my answers below.
>Четверг, 2 сентября 2021, 18:48 +03:00 от Igor Munkin <imun at 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 at tarantool.org wrote:
>> From: Oleg Babin < babinoleg at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20210902/c53de60b/attachment.htm>
More information about the Tarantool-patches
mailing list