Merge pull request #1758 from houqp/fixscheduler

Fix scheduler in UIManager

.gitignore

@@ -24,6 +24,7 @@ koreader-*.click
 l10n/*
 !l10n/Makefile
 !l10n/README.md
+i18n
 
 /.cproject
 /.project

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
+        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
-    until all_tasks_checked
-    self._execution_stack_dirty = false
+        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)

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)

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)

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)