From b1a1aeca0a6c3f967ce4ad58c0e1fc28e5b9a864 Mon Sep 17 00:00:00 2001 From: Qingping Hou Date: Fri, 25 Dec 2015 16:48:40 -0800 Subject: [PATCH 1/3] fix(task scheduler): many race conditions _checkTasks first get number of tasks in the stack and does a numeric for loop to go through each task. The problem is a task might call schedule or unschedule, which will reorder tasks in the stack. This will invalidate many of the table indexes used in the for loop. This patch turns the task stack into an ordered queue, so _checkTasks only pops one item out of the queue each time instead of setting up a for loop at the beginning. This should avoid the race condition mentioned above. --- frontend/ui/uimanager.lua | 122 ++++++++++++++++++++++++-------------- 1 file changed, 77 insertions(+), 45 deletions(-) diff --git a/frontend/ui/uimanager.lua b/frontend/ui/uimanager.lua index 6ac406710..18fbce235 100644 --- a/frontend/ui/uimanager.lua +++ b/frontend/ui/uimanager.lua @@ -8,6 +8,8 @@ local DEBUG = require("dbg") local _ = require("gettext") local ffi = require("ffi") +local MILLION = 1000000 + -- there is only one instance of this local UIManager = { -- trigger a full refresh when counter reaches FULL_REFRESH_COUNT @@ -18,8 +20,8 @@ local UIManager = { _running = true, _window_stack = {}, - _execution_stack = {}, - _execution_stack_dirty = false, + _task_queue = {}, + _task_queue_dirty = false, _dirty = {}, _zeromqs = {}, _refresh_stack = {}, @@ -136,22 +138,51 @@ function UIManager:close(widget, refreshtype, refreshregion) end end --- schedule an execution task +-- schedule an execution task, task queue is in ascendant order function UIManager:schedule(time, action) - table.insert(self._execution_stack, { time = time, action = action }) - self._execution_stack_dirty = true + local p, s, e = 1, 1, #self._task_queue + if e ~= 0 then + local us = time[1] * MILLION + time[2] + -- do a binary insert + repeat + p = math.floor(s + (e - s) / 2) + local ptime = self._task_queue[p].time + local ptus = ptime[1] * MILLION + ptime[2] + if us > ptus then + if s == e then + p = e + 1 + break + elseif s + 1 == e then + s = e + else + s = p + end + elseif us < ptus then + e = p + if s == e then + break + end + else + -- for fairness, it's better to make p+1 is strictly less than p + -- might want to revisit here in the future + break + end + until e < s + end + table.insert(self._task_queue, p, { time = time, action = action }) + self._task_queue_dirty = true end -- schedule task in a certain amount of seconds (fractions allowed) from now function UIManager:scheduleIn(seconds, action) local when = { util.gettime() } local s = math.floor(seconds) - local usecs = (seconds - s) * 1000000 + local usecs = (seconds - s) * MILLION when[1] = when[1] + s when[2] = when[2] + usecs - if when[2] > 1000000 then + if when[2] > MILLION then when[1] = when[1] + 1 - when[2] = when[2] - 1000000 + when[2] = when[2] - MILLION end self:schedule(when, action) end @@ -163,11 +194,11 @@ end -- UIManager:scheduleIn(10, self.anonymousFunction) -- UIManager:unschedule(self.anonymousFunction) function UIManager:unschedule(action) - for i = #self._execution_stack, 1, -1 do - local task = self._execution_stack[i] + for i = #self._task_queue, 1, -1 do + local task = self._task_queue[i] if task.action == action then -- remove from table - table.remove(self._execution_stack, i) + table.remove(self._task_queue, i) end end end @@ -245,14 +276,15 @@ end -- signal to quit function UIManager:quit() - DEBUG("quit uimanager") + DEBUG("quiting uimanager") + self._task_queue_dirty = false self._running = false self._run_forever = nil for i = #self._window_stack, 1, -1 do table.remove(self._window_stack, i) end - for i = #self._execution_stack, 1, -1 do - table.remove(self._execution_stack, i) + for i = #self._task_queue, 1, -1 do + table.remove(self._task_queue, i) end for i = #self._zeromqs, 1, -1 do self._zeromqs[i]:stop() @@ -287,35 +319,36 @@ end function UIManager:_checkTasks() local now = { util.gettime() } - - -- check if we have timed events in our queue and search next one + local now_us = now[1] * MILLION + now[2] local wait_until = nil - local all_tasks_checked - repeat - all_tasks_checked = true - for i = #self._execution_stack, 1, -1 do - local task = self._execution_stack[i] - if not task.time - or task.time[1] < now[1] - or task.time[1] == now[1] and task.time[2] < now[2] then - -- task is pending to be executed right now. do it. - task.action() - -- and remove from table - table.remove(self._execution_stack, i) - -- start loop again, since new tasks might be on the - -- queue now - all_tasks_checked = false - elseif not wait_until - or wait_until[1] > task.time[1] - or wait_until[1] == task.time[1] and wait_until[2] > task.time[2] then - -- task is to be run in the future _and_ is scheduled - -- earlier than the tasks we looked at already - -- so adjust to the currently examined task instead. - wait_until = task.time - end + + while true do + st_size = #self._task_queue + if st_size == 0 then + -- all tasks checked + break end - until all_tasks_checked - self._execution_stack_dirty = false + local task = self._task_queue[1] + local task_us = 0 + if task.time ~= nil then + task_us = task.time[1] * MILLION + task.time[2] + end + if task_us <= now_us then + -- remove from table + table.remove(self._task_queue, 1) + -- task is pending to be executed right now. do it. + -- NOTE: be careful that task.action() might modify + -- _task_queue here. So need to avoid race condition + task.action() + else + -- queue is sorted in ascendant order, safe to assume all items + -- are future tasks for now + wait_until = task.time + break + end + end + + self._task_queue_dirty = false return wait_until, now end @@ -451,9 +484,8 @@ function UIManager:handleInput() -- for input events: repeat wait_until, now = self:_checkTasks() - --DEBUG("---------------------------------------------------") - --DEBUG("exec stack", self._execution_stack) + --DEBUG("exec stack", self._task_queue) --DEBUG("window stack", self._window_stack) --DEBUG("dirty stack", self._dirty) --DEBUG("---------------------------------------------------") @@ -466,7 +498,7 @@ function UIManager:handleInput() end self:_repaint() - until not self._execution_stack_dirty + until not self._task_queue_dirty -- wait for next event -- note that we will skip that if we have tasks that are ready to run @@ -490,7 +522,7 @@ function UIManager:handleInput() local wait_for = { s = wait_until[1] - now[1], us = wait_until[2] - now[2] } if wait_for.us < 0 then wait_for.s = wait_for.s - 1 - wait_for.us = 1000000 + wait_for.us + wait_for.us = MILLION + wait_for.us end -- wait until next task is pending input_event = Input:waitEvent(wait_for.us, wait_for.s) From ab98097e725ae6adef1a861427628029f970514b Mon Sep 17 00:00:00 2001 From: Qingping Hou Date: Sat, 26 Dec 2015 12:52:07 -0800 Subject: [PATCH 2/3] add tests for scheduler --- .gitignore | 1 + spec/unit/uimanager_bench.lua | 49 ++++++++++++ spec/unit/uimanager_spec.lua | 137 ++++++++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+) create mode 100644 spec/unit/uimanager_bench.lua create mode 100644 spec/unit/uimanager_spec.lua diff --git a/.gitignore b/.gitignore index fa36f444f..3313fb28c 100644 --- a/.gitignore +++ b/.gitignore @@ -24,6 +24,7 @@ koreader-*.click l10n/* !l10n/Makefile !l10n/README.md +i18n /.cproject /.project diff --git a/spec/unit/uimanager_bench.lua b/spec/unit/uimanager_bench.lua new file mode 100644 index 000000000..2eafcd64a --- /dev/null +++ b/spec/unit/uimanager_bench.lua @@ -0,0 +1,49 @@ +require("commonrequire") +local util = require("ffi/util") +local UIManager = require("ui/uimanager") + +local noop = function() end + +describe("UIManager checkTasks benchmark", function() + local now = { util.gettime() } + UIManager:quit() + UIManager._task_queue = {} + + for i=1,1000000 do + table.insert( + UIManager._task_queue, + { time = { now[1] + 10000+i, now[2] }, action = noop } + ) + end + + -- for i=1,1000 do + wait_until, now = UIManager:_checkTasks() + -- end +end) + +describe("UIManager schedule benchmark", function() + local now = { util.gettime() } + UIManager:quit() + UIManager._task_queue = {} + for i=1,100000 do + UIManager:schedule({ now[1] + i, now[2] }, noop) + end +end) + +describe("UIManager unschedule benchmark", function() + local now = { util.gettime() } + UIManager:quit() + UIManager._task_queue = {} + + for i=1,1000 do + table.insert( + UIManager._task_queue, + { time = { now[1] + 10000+i, now[2] }, action = 'a' } + ) + end + + for i=1,1000 do + UIManager:schedule(now, noop) + UIManager:unschedule(noop) + end +end) diff --git a/spec/unit/uimanager_spec.lua b/spec/unit/uimanager_spec.lua new file mode 100644 index 000000000..b7b170bcc --- /dev/null +++ b/spec/unit/uimanager_spec.lua @@ -0,0 +1,137 @@ +require("commonrequire") +local util = require("ffi/util") +local UIManager = require("ui/uimanager") +local DEBUG = require("dbg") +DEBUG:turnOn() + +describe("UIManager spec", function() + local noop = function() end + + it("should consume due tasks", function() + local now = { util.gettime() } + local future = { now[1] + 60000, now[2] } + local future2 = {future[1] + 5, future[2]} + UIManager:quit() + UIManager._task_queue = { + { time = {now[1] - 10, now[2] }, action = noop }, + { time = {now[1], now[2] - 5 }, action = noop }, + { time = now, action = noop }, + { time = future, action = noop }, + { time = future2, action = noop }, + } + UIManager:_checkTasks() + assert.are.same(#UIManager._task_queue, 2) + assert.are.same(UIManager._task_queue[1].time, future) + assert.are.same(UIManager._task_queue[2].time, future2) + end) + + it("should calcualte wait_until properly in checkTasks routine", function() + local now = { util.gettime() } + local future = { now[1] + 60000, now[2] } + UIManager:quit() + UIManager._task_queue = { + { time = {now[1] - 10, now[2] }, action = noop }, + { time = {now[1], now[2] - 5 }, action = noop }, + { time = now, action = noop }, + { time = future, action = noop }, + { time = {future[1] + 5, future[2]}, action = noop }, + } + wait_until, now = UIManager:_checkTasks() + assert.are.same(wait_until, future) + end) + + it("should return nil wait_until properly in checkTasks routine", function() + local now = { util.gettime() } + UIManager:quit() + UIManager._task_queue = { + { time = {now[1] - 10, now[2] }, action = noop }, + { time = {now[1], now[2] - 5 }, action = noop }, + { time = now, action = noop }, + } + wait_until, now = UIManager:_checkTasks() + assert.are.same(wait_until, nil) + end) + + it("should insert new task properly in empty task queue", function() + local now = { util.gettime() } + UIManager:quit() + UIManager._task_queue = {} + assert.are.same(0, #UIManager._task_queue) + UIManager:scheduleIn(50, 'foo') + assert.are.same(1, #UIManager._task_queue) + assert.are.same(UIManager._task_queue[1].action, 'foo') + end) + + it("should insert new task properly in single task queue", function() + local now = { util.gettime() } + local future = { now[1]+10000, now[2] } + UIManager:quit() + UIManager._task_queue = { + { time = future, action = '1' }, + } + assert.are.same(1, #UIManager._task_queue) + UIManager:scheduleIn(150, 'quz') + assert.are.same(2, #UIManager._task_queue) + assert.are.same(UIManager._task_queue[1].action, 'quz') + + UIManager:quit() + UIManager._task_queue = { + { time = now, action = '1' }, + } + assert.are.same(1, #UIManager._task_queue) + UIManager:scheduleIn(150, 'foo') + assert.are.same(2, #UIManager._task_queue) + assert.are.same(UIManager._task_queue[2].action, 'foo') + UIManager:scheduleIn(155, 'bar') + assert.are.same(3, #UIManager._task_queue) + assert.are.same(UIManager._task_queue[3].action, 'bar') + end) + + it("should insert new task in ascendant order", function() + local now = { util.gettime() } + local noop1 = function() end + UIManager:quit() + UIManager._task_queue = { + { time = {now[1] - 10, now[2] }, action = '1' }, + { time = {now[1], now[2] - 5 }, action = '2' }, + { time = now, action = '3' }, + } + -- insert into the tail slot + UIManager:scheduleIn(10, 'foo') + assert.are.same('foo', UIManager._task_queue[4].action) + -- insert into the second slot + UIManager:schedule({now[1]-5, now[2]}, 'bar') + assert.are.same('bar', UIManager._task_queue[2].action) + -- insert into the head slot + UIManager:schedule({now[1]-15, now[2]}, 'baz') + assert.are.same('baz', UIManager._task_queue[1].action) + -- insert into the last second slot + UIManager:scheduleIn(5, 'qux') + assert.are.same('qux', UIManager._task_queue[6].action) + -- insert into the middle slot + UIManager:schedule({now[1], now[2]-1}, 'quux') + assert.are.same('quux', UIManager._task_queue[5].action) + end) + + it("should not have race between unschedule and _checkTasks", function() + local now = { util.gettime() } + local run_count = 0 + local task_to_remove = function() + run_count = run_count + 1 + end + UIManager:quit() + UIManager._task_queue = { + { time = { now[1], now[2]-5 }, action = task_to_remove }, + { + time = { now[1]-10, now[2] }, + action = function() + run_count = run_count + 1 + UIManager:unschedule(task_to_remove) + end + }, + { time = now, action = task_to_remove }, + } + UIManager:_checkTasks() + assert.are.same(run_count, 2) + end) +end) From 5aefb41631dd1782aeb9dda438abec88d1338d43 Mon Sep 17 00:00:00 2001 From: Qingping Hou Date: Sat, 26 Dec 2015 14:20:51 -0800 Subject: [PATCH 3/3] fix readerfooter spec --- spec/unit/readerfooter_spec.lua | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/unit/readerfooter_spec.lua b/spec/unit/readerfooter_spec.lua index d9b9416ea..24e0ecaf0 100644 --- a/spec/unit/readerfooter_spec.lua +++ b/spec/unit/readerfooter_spec.lua @@ -9,10 +9,11 @@ describe("Readerfooter module", function() local readerui = ReaderUI:new{ document = DocumentRegistry:openDocument(sample_epub), } + readerui.view.footer.settings.page_progress = true readerui.view.footer.settings.all_at_once = true readerui.view.footer:updateFooterPage() timeinfo = readerui.view.footer:getTimeInfo() - assert.are.same('B:0% | '..timeinfo..' | => 0 | R:100% | TB: 00:00 | TC: 00:00', + assert.are.same('B:0% | '..timeinfo..' | 1 / 1 | => 0 | R:100% | TB: 00:00 | TC: 00:00', readerui.view.footer.progress_text.text) end) @@ -21,10 +22,11 @@ describe("Readerfooter module", function() local readerui = ReaderUI:new{ document = DocumentRegistry:openDocument(sample_pdf), } + readerui.view.footer.settings.page_progress = true readerui.view.footer.settings.all_at_once = true readerui.view.footer:updateFooterPage() timeinfo = readerui.view.footer:getTimeInfo() - assert.are.same('B:0% | '..timeinfo..' | => 1 | R:50% | TB: na | TC: na', + assert.are.same('B:0% | '..timeinfo..' | 1 / 2 | => 1 | R:50% | TB: na | TC: na', readerui.view.footer.progress_text.text) end) end)