[UIManager] Reverse order of _task_queue (#9706)

frontend/ui/uimanager.lua

@@ -247,10 +247,10 @@ function UIManager:close(widget, refreshtype, refreshregion, refreshdither)
     end
 end
 
--- Schedule an execution task; task queue is in ascending order
+-- Schedule an execution task; task queue is in descending order
 function UIManager:schedule(sched_time, action, ...)
     local lo, hi = 1, #self._task_queue
-    -- Rightmost binary insertion
+    -- Leftmost binary insertion
     while lo <= hi do
         -- NOTE: We should be (mostly) free from overflow here, thanks to LuaJIT's BitOp semantics.
         --       For more fun details about this particular overflow,
@@ -259,10 +259,10 @@ function UIManager:schedule(sched_time, action, ...)
         --       c.f., https://reprog.wordpress.com/2010/04/19/are-you-one-of-the-10-percent/
         local mid = bit.rshift(lo + hi, 1)
         local mid_time = self._task_queue[mid].time
-        if sched_time >= mid_time then
+        if mid_time <= sched_time then
-            lo = mid + 1
-        else
             hi = mid - 1
+        else
+            lo = mid + 1
         end
     end
 
@@ -941,10 +941,9 @@ end
 --]]
 
 function UIManager:getNextTaskTime()
-    if self._task_queue[1] then
+    local next_task = self._task_queue[#self._task_queue]
-        return self._task_queue[1].time - time:now()
+    if next_task then
-    else
+        return next_task.time - time:now()
-        return nil
     end
 end
 
@@ -956,17 +955,17 @@ function UIManager:_checkTasks()
     -- Flipping this switch ensures we'll consume all such tasks *before* yielding to input polling.
     self._task_queue_dirty = false
     while self._task_queue[1] do
-        local task_time = self._task_queue[1].time
+        local task_time = self._task_queue[#self._task_queue].time
         if task_time <= self._now then
-            -- Pop the upcoming task, as it is due for execution...
+            -- Remove the upcoming task, as it is due for execution...
-            local task = table.remove(self._task_queue, 1)
+            local task =  table.remove(self._task_queue)
             -- ...so do it now.
             -- NOTE: Said task's action might modify _task_queue.
             --       To avoid race conditions and catch new upcoming tasks during this call,
             --       we repeatedly check the head of the queue (c.f., #1758).
             task.action(unpack(task.args))
         else
-            -- As the queue is sorted in ascending order, it's safe to assume all items are currently future tasks.
+            -- As the queue is sorted in descending order, it's safe to assume all items are currently future tasks.
             wait_until = task_time
             break
         end

spec/unit/readerfooter_spec.lua

@@ -435,7 +435,7 @@ describe("Readerfooter module", function()
         os.remove(DocSettings:getHistoryPath(sample_epub))
         UIManager:quit()
 
-        assert.are.same({}, UIManager._task_queue)
+        assert.are.same(0, #UIManager._task_queue)
 
         local settings = G_reader_settings:readSetting("footer")
         settings.auto_refresh_time = true
@@ -473,7 +473,7 @@ describe("Readerfooter module", function()
         os.remove(DocSettings:getHistoryPath(sample_epub))
         UIManager:quit()
 
-        assert.are.same({}, UIManager._task_queue)
+        assert.are.same(0, #UIManager._task_queue)
 
         local settings = G_reader_settings:readSetting("footer")
         settings.disabled = true
@@ -503,7 +503,7 @@ describe("Readerfooter module", function()
         os.remove(DocSettings:getHistoryPath(sample_pdf))
         UIManager:quit()
 
-        assert.are.same({}, UIManager._task_queue)
+        assert.are.same(0, #UIManager._task_queue)
 
         local settings = G_reader_settings:readSetting("footer")
         settings.disabled = false
@@ -561,7 +561,7 @@ describe("Readerfooter module", function()
         os.remove(DocSettings:getHistoryPath(sample_pdf))
         UIManager:quit()
 
-        assert.are.same({}, UIManager._task_queue)
+        assert.are.same(0, #UIManager._task_queue)
 
         G_reader_settings:saveSetting("reader_footer_mode", 1)
         -- default settings
@@ -602,7 +602,7 @@ describe("Readerfooter module", function()
         os.remove(DocSettings:getHistoryPath(sample_pdf))
         UIManager:quit()
 
-        assert.are.same({}, UIManager._task_queue)
+        assert.are.same(0, #UIManager._task_queue)
 
         local settings = G_reader_settings:readSetting("footer")
         settings.all_at_once = true
@@ -640,7 +640,7 @@ describe("Readerfooter module", function()
         os.remove(DocSettings:getHistoryPath(sample_pdf))
         UIManager:quit()
 
-        assert.are.same({}, UIManager._task_queue)
+        assert.are.same(0, #UIManager._task_queue)
 
         G_reader_settings:saveSetting("reader_footer_mode", 0)
         local settings = G_reader_settings:readSetting("footer")
@@ -666,7 +666,7 @@ describe("Readerfooter module", function()
         os.remove(DocSettings:getHistoryPath(sample_epub))
         UIManager:quit()
 
-        assert.are.same({}, UIManager._task_queue)
+        assert.are.same(0, #UIManager._task_queue)
 
         local settings = G_reader_settings:readSetting("footer")
         settings.battery = false

spec/unit/taskqueue_bench.lua

@@ -0,0 +1,77 @@
+require("commonrequire")
+
+local UIManager = require("ui/uimanager")
+
+local time = require("ui/time")
+
+local NB_TESTS = 40000
+local noop = function() end
+
+describe("UIManager simple checkTasks and scheduling benchmark", function()
+    local now = time.now()
+    local wait_until -- luacheck: no unused
+    UIManager:quit()
+    UIManager._task_queue = {}
+
+    -- use schedule here, to be agnostic of the _task_queue order (ascending, descending).
+    for i=1, NB_TESTS/2 do
+        UIManager:schedule(now + i, noop)
+        UIManager:schedule(now + NB_TESTS - i, noop)
+    end
+
+    for i=1, NB_TESTS do
+        wait_until, now = UIManager:_checkTasks() -- luacheck: no unused
+    end
+end)
+
+describe("UIManager more advanced checkTasks and scheduling benchmark", function()
+    -- This BM is doing schedulings like the are done in real usage
+    -- with autosuspend, autodim, autowarmth and friends.
+    -- Additional _checkTask is called to better simulate bench this too.
+    local wait_until -- luacheck: no unused
+
+    local now = time.now()
+    UIManager:quit()
+
+    local function standby_dummy() end
+    local function autowarmth_dummy() end
+    local function dimmer_dummy() end
+
+    local function someTaps()
+        for j = 1,10 do
+            -- insert some random times for entering standby
+            UIManager:schedule(now + time.s(j), standby_dummy) -- standby
+            UIManager:unschedule(standby_dummy)
+        end
+    end
+
+    for i=1, NB_TESTS do
+        UIManager._task_queue = {}
+        UIManager:schedule(now + time.s(24*60*60), noop) -- shutdown
+        UIManager:schedule(now + time.s(15*60*60), noop) -- sleep
+        UIManager:schedule(now + time.s(55), noop) -- footer refresh
+        UIManager:schedule(now + time.s(130), noop) -- something
+        UIManager:schedule(now + time.s(10), noop) -- something else
+
+        for j = 1,5 do
+            someTaps()
+
+            for k = 1, 10 do
+                UIManager:schedule(now + time.s(k), standby_dummy) -- standby
+            end
+            now = now + 10
+
+            -- consume the last 10 standby_dummy, plus 10 checks.
+            for k = 1, 20 do
+                wait_until, now = UIManager:_checkTasks() -- luacheck: no unused
+            end
+
+            UIManager:schedule(now + time.s(15*60), autowarmth_dummy) -- autowarmth
+            UIManager:schedule(now + time.s(180), dimmer_dummy) -- dimmer
+
+            now = now + 30
+            UIManager:unschedule(dimmer_dummy)
+            UIManager:unschedule(autowarmth_dummy) -- remove autowarmth
+        end
+    end
+end)

spec/unit/uimanager_bench.lua

@@ -10,11 +10,11 @@ local noop = function() end
 local function check()
     for i = 1, #UIManager._task_queue-1 do
         -- test for wrongly inserted time
-        assert.is_true(UIManager._task_queue[i].time <= UIManager._task_queue[i+1].time,
+        assert.is_true(UIManager._task_queue[i].time >= UIManager._task_queue[i+1].time,
             "time wrongly sorted")
         if UIManager._task_queue[i].time == UIManager._task_queue[i+1].time then
             -- for same time, test if later inserted action is after a former action
-            assert.is_true(UIManager._task_queue[i].action <= UIManager._task_queue[i+1].action,
+            assert.is_true(UIManager._task_queue[i].action >= UIManager._task_queue[i+1].action,
                 "ragnarock")
         end
     end
@@ -24,9 +24,8 @@ describe("UIManager checkTasks benchmark", function()
     local now = time.now()
     local wait_until -- luacheck: no unused
     UIManager:quit()
-    UIManager._task_queue = {}
 
-    for i=1, NB_TESTS do
+    for i= NB_TESTS, 1, -1 do
         table.insert(
             UIManager._task_queue,
             { time = now + i, action = noop, args = {} }
@@ -41,16 +40,14 @@ end)
 describe("UIManager schedule simple benchmark", function()
     local now = time.now()
     UIManager:quit()
-    UIManager._task_queue = {}
 
-    -- Insert tasks at the beginning and at the end of the _task_queue
     for i=1, NB_TESTS/2 do
         UIManager:schedule(now + i, noop)
         UIManager:schedule(now + NB_TESTS - i, noop)
     end
 end)
 
-describe("UIManager schedule more sophiticated benchmark", function()
+describe("UIManager more sophisticated schedule benchmark", function()
     -- This BM is doing schedulings like the are done in real usage
     -- with autosuspend, autodim, autowarmth and friends.
     local now = time.now()
@@ -147,17 +144,18 @@ end)
 describe("UIManager unschedule benchmark", function()
     local now = time.now()
     UIManager:quit()
-    UIManager._task_queue = {}
 
-    for i=1, NB_TESTS do
+    for i=NB_TESTS, 1, -1 do
         table.insert(
             UIManager._task_queue,
             { time = now + i, action = 'a', args={} }
         )
     end
 
-    for i=1, NB_TESTS do
+    for i=1, NB_TESTS/2 do
         UIManager:schedule(now + i, noop)
         UIManager:unschedule(noop)
+        UIManager:schedule(now + NB_TESTS - i, noop)
+        UIManager:unschedule(noop)
     end
 end)

spec/unit/uimanager_spec.lua

@@ -15,16 +15,17 @@ describe("UIManager spec", function()
         local future2 = future + time.s(5)
         UIManager:quit()
         UIManager._task_queue = {
-            { time = now - time.s(10), action = noop, args = {} },
-            { time = now - time.us(5), action = noop, args = {} },
-            { time = now, action = noop, args = {} },
-            { time = future, action = noop, args = {} },
             { time = future2, action = noop, args = {} },
+            { time = future, action = noop, args = {} },
+            { time = now, action = noop, args = {} },
+            { time = now - time.us(5), action = noop, args = {} },
+            { time = now - time.s(10), action = noop, args = {} },
         }
+
         UIManager:_checkTasks()
-        assert.are.same(2, #UIManager._task_queue, 2)
+        assert.are.same(2, #UIManager._task_queue)
-        assert.are.same(future, UIManager._task_queue[1].time)
+        assert.are.same(future, UIManager._task_queue[2].time)
-        assert.are.same(future2, UIManager._task_queue[2].time)
+        assert.are.same(future2, UIManager._task_queue[1].time)
     end)
 
     it("should calcualte wait_until properly in checkTasks routine", function()
@@ -32,11 +33,12 @@ describe("UIManager spec", function()
         local future_time = now + time.s(60000)
         UIManager:quit()
         UIManager._task_queue = {
-            { time = now - time.s(10), action = noop, args = {} },
-            { time = now - time.us(5), action = noop, args = {} },
-            { time = now, action = noop, args = {} },
             { time = future_time, action = noop, args = {} },
+            { time = now, action = noop, args = {} },
+            { time = now - time.us(5), action = noop, args = {} },
+            { time = now - time.s(10), action = noop, args = {} },
         }
+
         wait_until, now = UIManager:_checkTasks()
         assert.are.same(future_time, wait_until)
     end)
@@ -45,10 +47,11 @@ describe("UIManager spec", function()
         now = time.now()
         UIManager:quit()
         UIManager._task_queue = {
-            { time = now - time.s(10), action = noop, args = {} },
-            { time = now - time.us(5), action = noop, args = {} },
             { time = now, action = noop, args = {} },
+            { time = now - time.us(5), action = noop, args = {} },
+            { time = now - time.s(10), action = noop, args = {} },
         }
+
         wait_until, now = UIManager:_checkTasks()
         assert.are.same(nil, wait_until)
     end)
@@ -56,7 +59,6 @@ describe("UIManager spec", function()
     it("should insert new task properly in empty task queue", function()
         now = time.now()
         UIManager:quit()
-        UIManager._task_queue = {}
         assert.are.same(0, #UIManager._task_queue)
         UIManager:scheduleIn(50, 'foo')
         assert.are.same(1, #UIManager._task_queue)
@@ -70,113 +72,120 @@ describe("UIManager spec", function()
         UIManager._task_queue = {
             { time = future_time, action = '1', args = {} },
         }
+
         assert.are.same(1, #UIManager._task_queue)
         UIManager:scheduleIn(150, 'quz')
         assert.are.same(2, #UIManager._task_queue)
-        assert.are.same('quz', UIManager._task_queue[1].action)
+        assert.are.same('quz', UIManager._task_queue[2].action)
 
         UIManager:quit()
         UIManager._task_queue = {
             { time = now, action = '1', args = {} },
         }
+
         assert.are.same(1, #UIManager._task_queue)
         UIManager:scheduleIn(150, 'foo')
         assert.are.same(2, #UIManager._task_queue)
-        assert.are.same('foo', UIManager._task_queue[2].action)
+        assert.are.same('foo', UIManager._task_queue[1].action)
         UIManager:scheduleIn(155, 'bar')
         assert.are.same(3, #UIManager._task_queue)
-        assert.are.same('bar', UIManager._task_queue[3].action)
+        assert.are.same('bar', UIManager._task_queue[1].action)
     end)
 
-    it("should insert new task in ascendant order", function()
+    it("should insert new task in descendant order", function()
         now = time.now()
         UIManager:quit()
         UIManager._task_queue = {
-            { time = now - time.s(10), action = '1', args = {} },
-            { time = now - time.us(5), action = '2', args = {} },
             { time = now, action = '3', args = {} },
+            { time = now - time.us(5), action = '2', args = {} },
+            { time = now - time.s(10), action = '1', args = {} },
         }
+
         -- insert into the tail slot
         UIManager:scheduleIn(10, 'foo')
-        assert.are.same('foo', UIManager._task_queue[4].action)
+        assert.are.same('foo', UIManager._task_queue[1].action)
         -- insert into the second slot
         UIManager:schedule(now - time.s(5), 'bar')
-        assert.are.same('bar', UIManager._task_queue[2].action)
+        assert.are.same('bar', UIManager._task_queue[4].action)
         -- insert into the head slot
         UIManager:schedule(now - time.s(15), 'baz')
-        assert.are.same('baz', UIManager._task_queue[1].action)
+        assert.are.same('baz', UIManager._task_queue[6].action)
         -- insert into the last second slot
         UIManager:scheduleIn(5, 'qux')
-        assert.are.same('qux', UIManager._task_queue[6].action)
+        assert.are.same('qux', UIManager._task_queue[2].action)
         -- insert into the middle slot
         UIManager:schedule(now - time.us(1), 'quux')
-        assert.are.same('quux', UIManager._task_queue[5].action)
+        assert.are.same('quux', UIManager._task_queue[4].action)
     end)
 
-    it("should insert new tasks with same times after existing tasks", function()
+    it("should insert new tasks with same times before existing tasks", function()
+        local ffiutil = require("ffi/util")
+
         now = time.now()
         UIManager:quit()
-        UIManager._task_queue = {}
 
         -- insert task "5s" between "now" and "10s"
         UIManager:schedule(now, "now");
         assert.are.same("now", UIManager._task_queue[1].action)
         UIManager:schedule(now + time.s(10), "10s");
-        assert.are.same("10s", UIManager._task_queue[2].action)
+        assert.are.same("10s", UIManager._task_queue[1].action)
         UIManager:schedule(now + time.s(5), "5s");
         assert.are.same("5s", UIManager._task_queue[2].action)
 
-        -- insert task at the end after "10s"
+        -- insert task in place of "10s", as it'll expire shortly afer "10s"
-        -- NOTE: Can't use 10, as time.now, which is used internally, may or may not have moved,
+        -- NOTE: Can't use this here right now, as time.now, which is used internally,
-        --       depending on host's performance and clock granularity (especially if host is fast and/or COARSE is available).
+        -- may or may not have moved, depending on host's performance and clock granularity
-        UIManager:scheduleIn(11, 'foo')
+        -- (especially if host is fast and/or COARSE is available).
-        assert.are.same('foo', UIManager._task_queue[4].action)
+        -- But a short wait fixes this here.
+        ffiutil.usleep(1000)
+        UIManager:scheduleIn(10, 'foo') -- is a bit later than "10s", as time.now() is used internally
+        assert.are.same('foo', UIManager._task_queue[1].action)
 
-        -- insert task at the second last position after "10s"
+        -- insert task in place of "10s", which was just shifted by foo
         UIManager:schedule(now + time.s(10), 'bar')
-        assert.are.same('bar', UIManager._task_queue[4].action)
+        assert.are.same('bar', UIManager._task_queue[2].action)
 
-        -- insert task at the second last position after "bar"
+        -- insert task in place of "bar"
         UIManager:schedule(now + time.s(10), 'baz')
-        assert.are.same('baz', UIManager._task_queue[5].action)
+        assert.are.same('baz', UIManager._task_queue[2].action)
 
-        -- insert task after "5s"
+        -- insert task in place of "5s"
         UIManager:schedule(now + time.s(5), 'nix')
-        assert.are.same('nix', UIManager._task_queue[3].action)
+        assert.are.same('nix', UIManager._task_queue[5].action)
-        -- "barba" is later than "nix" anyway
+        -- "barba" replaces "nix"
         UIManager:scheduleIn(5, 'barba') -- is a bit later than "5s", as time.now() is used internally
-        assert.are.same('barba', UIManager._task_queue[4].action)
+        assert.are.same('barba', UIManager._task_queue[5].action)
 
-        -- "mama" is sheduled now and inserted after "now"
+        -- "mama is sheduled now and as such inserted in "now"'s place
         UIManager:schedule(now, 'mama')
-        assert.are.same('mama', UIManager._task_queue[2].action)
+        assert.are.same('mama', UIManager._task_queue[8].action)
 
-        -- "papa" is shortly after "now"
+        -- "papa" is shortly after "now", so inserted in its place
         -- NOTE: For the same reason as above, test this last, as time.now may not have moved...
         UIManager:nextTick('papa') -- is a bit later than "now"
-        assert.are.same('papa', UIManager._task_queue[3].action)
+        assert.are.same('papa', UIManager._task_queue[8].action)
 
-        -- "letta" is shortly after "papa"
+        -- "letta" is shortly after "papa", so inserted in its place
         UIManager:tickAfterNext('letta')
-        assert.are.same("function", type(UIManager._task_queue[4].action))
+        assert.are.same("function", type(UIManager._task_queue[8].action))
-
     end)
 
     it("should unschedule all the tasks with the same action", function()
         now = time.now()
         UIManager:quit()
         UIManager._task_queue = {
-            { time = now - time.s(15), action = '3', args = {} },
-            { time = now - time.s(10), action = '1', args = {} },
-            { time = now - time.us(6), action = '3', args = {} },
-            { time = now - time.us(5), action = '2', args = {} },
             { time = now, action = '3', args = {} },
+            { time = now - time.us(5), action = '2', args = {} },
+            { time = now - time.us(6), action = '3', args = {} },
+            { time = now - time.s(10), action = '1', args = {} },
+            { time = now - time.s(15), action = '3', args = {} },
         }
+
         -- insert into the tail slot
         UIManager:unschedule('3')
         assert.are.same({
-            { time = now - time.s(10), action = '1', args = {} },
             { time = now - time.us(5), action = '2', args = {} },
+            { time = now - time.s(10), action = '1', args = {} },
         }, UIManager._task_queue)
     end)
 
@@ -188,7 +197,8 @@ describe("UIManager spec", function()
         end
         UIManager:quit()
         UIManager._task_queue = {
-            { time = now - time.s(15), action = task_to_remove, args = {} }, -- this will be called
+            { time = now, action = task_to_remove, args = {} }, -- this will be removed
+            { time = now - time.us(5), action = task_to_remove, args = {} }, -- this will be removed
             {
                 time = now - time.s(10),
                 action = function() -- this will be called, too
@@ -197,9 +207,9 @@ describe("UIManager spec", function()
                 end,
                 args = {},
             },
-            { time = now - time.us(5), action = task_to_remove, args = {} }, -- this will be removed
+            { time = now - time.s(15), action = task_to_remove, args = {} }, -- this will be called
-            { time = now, action = task_to_remove, args = {} }, -- this will be removed
         }
+
         UIManager:_checkTasks()
         assert.are.same(2, run_count)
     end)