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

[Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for your review. My answers below.

On 25.08.2021 23:33, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> See 5 comments below.
>
>> 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";
>>   
>>   /**
>> - * @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.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 = (struct fiber *) event;
> 1. In new code we do not use whitespaces after unary operators,
> including type casts. But here you don't need a cast. void * is
> implicitly compatible with any other pointer type in C.

Fixed

>> +	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, 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)
>>   {
>> -	/*
>> -	 * 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 ref = f->storage.lua.ref;
>> +	if (ref <= 0) {
>> +		struct trigger *t = (struct trigger *)malloc(sizeof(*t));
> 2. No need to cast, in C it works as is.
Fixed.
>> +		if (t == NULL) {
>> +			diag_set(OutOfMemory, sizeof(*t), "malloc", "t");
>> +			luaT_error(L);
>> +		}
>> +		trigger_create(t, lbox_fiber_on_stop, NULL, (trigger_f0) free);
> 3. Should not be a whitespace after the cast.
Fixed.
>> +		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;
> 4. Did you consider pushing the fiber's pointer instead of its ID?
> And keep the fiber struct from deletion until it has no refs in
> Lua anymore. That would eliminate lookup in fiber hash on each attempt
> to access it. Also might make it simpler to return stuff like fiber.csw
> in Lua. I remember there was a problem with the fiber being deleted by
> the time you try to access its members. This would solve it.

I'll think about it. I guess it's possible but it's required more time 
and could be done

on the top of this patch.


>>   		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);
>>   }
>> @@ -703,8 +669,6 @@ lbox_fiber_storage(struct lua_State *L)
>>   			diag_set(OutOfMemory, sizeof(*t), "malloc", "t");
>>   			return luaT_error(L);
>>   		}
>> -		trigger_create(t, lbox_fiber_on_stop, NULL, (trigger_f0) free);
> 5. The trigger 't' now is not used and leaks.

Oh, I've missed it. Malloc is removed.


>> -		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;
>>

Diff:

diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 268ddf9cc..83a6b4850 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -99,7 +99,7 @@ static const char *fiberlib_name = "fiber";
  static int
  lbox_fiber_on_stop(struct trigger *trigger, void *event)
  {
-    struct fiber *f = (struct fiber *) 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);
@@ -122,12 +122,12 @@ lbox_pushfiber(struct lua_State *L, struct fiber *f)
  {
      int ref = f->storage.lua.ref;
      if (ref <= 0) {
-        struct trigger *t = (struct trigger *)malloc(sizeof(*t));
+        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_create(t, lbox_fiber_on_stop, NULL, (trigger_f0)free);
          trigger_add(&f->on_stop, t);

          uint64_t fid = f->fid;
@@ -663,12 +663,6 @@ 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) {
-        struct trigger *t = (struct trigger *)
-            malloc(sizeof(*t));
-        if (t == NULL) {
-            diag_set(OutOfMemory, sizeof(*t), "malloc", "t");
-            return luaT_error(L);
-        }
          lua_newtable(L); /* create local storage on demand */
          storage_ref = luaL_ref(L, LUA_REGISTRYINDEX);
          f->storage.lua.storage_ref = storage_ref;