Thanks for your review. See my answers below.   >Четверг, 2 сентября 2021, 18:48 +03:00 от Igor Munkin : >  >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 , 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"; >> > > > >> +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; >> + if (storage_ref > 0) { > >I don't like this. At first, 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 > 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 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) >> { > > > >> + 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 ). 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 * > > > >> -- >> 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  #include +#include  #include  #include  #include @@ -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