Re: [Tarantool-patches] [PATCH v2 1/3] fiber: introduce schedule_task() internal function

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the review!

On 19/03/2020 15:52, Igor Munkin wrote:
> Vlad,
> 
> Thanks for the patch! Please consider several minor comments I left.
> 
> On 03.03.20, Vladislav Shpilevoy wrote:
>> fiber._internal.schedule_task() is an API for a singleton fiber
>> worker object. It serves for not urgent delayed execution of
>> functions. Main purpose - schedule execution of a function, which
>> is going to yield, from a context, where a yield is not allowed.
>> Such as an FFI object's GC callback.
>>
>> It will be used by SWIM and by fio, whose destruction yields, but
>> they need to use ffi.gc hook, where a yield is not allowed.
> 
> Minor: AFAIR __gc metamethod[1] also obligues the one to avoid
> coroutine/fiber switch[2]. Thereby GC finalizer looks to be more
> accurate term here.

You know better. Changed 'ffi.gc hook' -> 'GC finalizer'.

>>
>> Part of #4727
>> ---
>>  src/lua/fiber.c         | 15 +++++++++
>>  src/lua/fiber.lua       | 60 ++++++++++++++++++++++++++++++++++
>>  test/app/fiber.result   | 72 +++++++++++++++++++++++++++++++++++++++++
>>  test/app/fiber.test.lua | 41 +++++++++++++++++++++++
>>  4 files changed, 188 insertions(+)
>>
>> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
>> index 575a020d0..ddf827ab6 100644
>> --- a/src/lua/fiber.c
>> +++ b/src/lua/fiber.c
>> @@ -828,6 +828,19 @@ lbox_fiber_set_joinable(struct lua_State *L)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * Alternative to fiber.sleep(infinite) which does not participate
>> + * in an event loop at all until an explicit wakeup. This is less
>> + * overhead. Useful for fibers sleeping most of the time.
>> + */
> 
> Minor: I guess <lbox_fiber_stall> and <fiber.stall> are good names for
> this API. Feel free to ignore.

Yeah, sounds much better.

====================
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index ddf827ab6..03b1b959c 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -834,7 +834,7 @@ lbox_fiber_set_joinable(struct lua_State *L)
  * overhead. Useful for fibers sleeping most of the time.
  */
 static int
-lbox_fiber_sleep_infinite(struct lua_State *L)
+lbox_fiber_stall(struct lua_State *L)
 {
 	(void) L;
 	fiber_yield();
@@ -879,7 +879,7 @@ static const struct luaL_Reg fiberlib[] = {
 	{"status", lbox_fiber_status},
 	{"name", lbox_fiber_name},
 	/* Internal functions, to hide in fiber.lua. */
-	{"sleep_infinite", lbox_fiber_sleep_infinite},
+	{"stall", lbox_fiber_stall},
 	{NULL, NULL}
 };
 
diff --git a/src/lua/fiber.lua b/src/lua/fiber.lua
index d0b765b60..fa53403d0 100644
--- a/src/lua/fiber.lua
+++ b/src/lua/fiber.lua
@@ -35,8 +35,8 @@ fiber.time64 = fiber_time64
 fiber.clock = fiber_clock
 fiber.clock64 = fiber_clock64
 
-local sleep_infinite = fiber.sleep_infinite
-fiber.sleep_infinite = nil
+local stall = fiber.stall
+fiber.stall = nil
 
 local worker_next_task = nil
 local worker_last_task = nil
@@ -56,7 +56,7 @@ local function worker_f()
             if task then
                 break
             end
-            sleep_infinite()
+            stall()
         end
         worker_next_task = task.next
         task.f(task.arg)
====================

>> diff --git a/src/lua/fiber.lua b/src/lua/fiber.lua
>> index 8712ee0d6..d0b765b60 100644
>> --- a/src/lua/fiber.lua
>> +++ b/src/lua/fiber.lua
>> @@ -34,4 +34,64 @@ fiber.time = fiber_time
>>  fiber.time64 = fiber_time64
>>  fiber.clock = fiber_clock
>>  fiber.clock64 = fiber_clock64
>> +
>> +local sleep_infinite = fiber.sleep_infinite
>> +fiber.sleep_infinite = nil
>> +
>> +local worker_next_task = nil
>> +local worker_last_task = nil
>> +local worker_fiber = nil
>> +
>> +--
>> +-- Worker is a singleton fiber for not urgent delayed execution of
>> +-- functions. Main purpose - schedule execution of a function,
>> +-- which is going to yield, from a context, where a yield is not
>> +-- allowed. Such as an FFI object's GC callback.
>> +--
>> +local function worker_f()
>> +    local task
>> +    while true do
>> +        while true do
>> +            task = worker_next_task
>> +            if task then
>> +                break
>> +            end
>> +            sleep_infinite()
>> +        end
>> +        worker_next_task = task.next
>> +        task.f(task.arg)
>> +        fiber.sleep(0)
>> +    end
>> +end
>> +
>> +local function worker_safe_f()
>> +    pcall(worker_f)
>> +    -- This fiber is probably canceled and now is not able to
>> +    -- sleep, create a new one.
> 
> Minor: I guess it worth mentioning the fact that <worker_f> doesn't
> return. It would definitely make the comment clearer.

Done.

====================
diff --git a/src/lua/fiber.lua b/src/lua/fiber.lua
index fa53403d0..89c17f63d 100644
--- a/src/lua/fiber.lua
+++ b/src/lua/fiber.lua
@@ -66,8 +66,9 @@ end
 
 local function worker_safe_f()
     pcall(worker_f)
-    -- This fiber is probably canceled and now is not able to
-    -- sleep, create a new one.
+    -- Worker_f never returns. If the execution is here, this
+    -- fiber is probably canceled and now is not able to sleep.
+    -- Create a new one.
     worker_fiber = fiber.new(worker_safe_f)
 end
====================

>> +    worker_fiber = fiber.new(worker_safe_f)
>> +end
>> +
>> +worker_fiber = fiber.new(worker_safe_f)
>> +
>> +local function worker_schedule_task(f, arg)
>> +    local task = {f = f, arg = arg}
>> +    if not worker_next_task then
>> +        worker_next_task = task
>> +    else
> 
> Minor: It would be great to add an assert here, since nothing except the
> current implementation prevents us from indexing a nil value. Feel free
> to ignore.
The assertion will explode with the same error at an attempt
to index nil - no information will be lost, but the assert
would be executed even in Release. Lets better omit it.

>> +        worker_last_task.next = task
>> +    end
>> +    worker_last_task = task
>> +    worker_fiber:wakeup()
>> +end
>> +
>> diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
>> index 6df210d9c..f10782d1f 100644
>> --- a/test/app/fiber.test.lua
>> +++ b/test/app/fiber.test.lua
>> @@ -688,6 +688,47 @@ tbl.time > 0
>>  fiber.top_disable()
>>  fiber.top()
>>  
>> +--
>> +-- fiber._internal.schedule_task() - API for internal usage for
>> +-- delayed execution of a function.
>> +--
>> +glob_arg = {}
>> +count = 0
>> +function task_f(arg)                                                            \
>> +    count = count + 1                                                           \
>> +    table.insert(glob_arg, arg)                                                 \
>> +    arg = arg + 1                                                               \
>> +    if arg <= 3 then                                                            \
>> +        fiber._internal.schedule_task(task_f, arg)                              \
>> +    else                                                                        \
>> +        fiber.self():cancel()                                                   \
>> +        error('Worker is broken')                                               \
>> +    end                                                                         \
>> +end
>> +for i = 1, 3 do                                                                 \
>> +    local csw1 = fiber.info()[fiber.id()].csw                                   \
>> +    fiber._internal.schedule_task(task_f, i)                                    \
>> +    local csw2 = fiber.info()[fiber.id()].csw                                   \
>> +    assert(csw1 == csw2 and csw1 ~= nil)                                        \
> 
> Minor: IMHO it's better to check that csw1 is a 'number'. Feel free to
> ignore.

I don't mind.

====================
diff --git a/test/app/fiber.result b/test/app/fiber.result
index bd60e1483..0e1ca815c 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1588,7 +1588,7 @@ for i = 1, 3 do
     local csw1 = fiber.info()[fiber.id()].csw                                   \
     fiber._internal.schedule_task(task_f, i)                                    \
     local csw2 = fiber.info()[fiber.id()].csw                                   \
-    assert(csw1 == csw2 and csw1 ~= nil)                                        \
+    assert(csw1 == csw2 and type(csw1) == 'number')                             \
 end
 ---
 ...
diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
index f10782d1f..f01144f49 100644
--- a/test/app/fiber.test.lua
+++ b/test/app/fiber.test.lua
@@ -709,7 +709,7 @@ for i = 1, 3 do
     local csw1 = fiber.info()[fiber.id()].csw                                   \
     fiber._internal.schedule_task(task_f, i)                                    \
     local csw2 = fiber.info()[fiber.id()].csw                                   \
-    assert(csw1 == csw2 and csw1 ~= nil)                                        \
+    assert(csw1 == csw2 and type(csw1) == 'number')                             \
 end
 old_count = count
 test_run:wait_cond(function()                                                   \
====================