[Tarantool-patches] [PATCH v2 1/3] fiber: introduce schedule_task() internal function
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Mar 20 03:00:34 MSK 2020
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() \
====================
More information about the Tarantool-patches
mailing list