From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4CE60469719 for ; Fri, 20 Mar 2020 03:00:37 +0300 (MSK) References: <136873bd559abc466dc2761006f7954fea0c1a68.1583191602.git.v.shpilevoy@tarantool.org> <20200319145257.GJ6392@tarantool.org> From: Vladislav Shpilevoy Message-ID: <9d071039-e834-10d1-62b3-a562abea6f5b@tarantool.org> Date: Fri, 20 Mar 2020 01:00:34 +0100 MIME-Version: 1.0 In-Reply-To: <20200319145257.GJ6392@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 1/3] fiber: introduce schedule_task() internal function List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.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 and 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 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() \ ====================