From 07748b0999a9f390e46c1626a02f6fa3e49d5d2e Mon Sep 17 00:00:00 2001 From: zwim <36999612+zwim@users.noreply.github.com> Date: Wed, 2 Nov 2022 20:08:15 +0100 Subject: [PATCH] [UIManager] Reverse order of _task_queue (#9706) --- frontend/ui/uimanager.lua | 25 ++++--- spec/unit/readerfooter_spec.lua | 14 ++-- spec/unit/taskqueue_bench.lua | 77 ++++++++++++++++++++ spec/unit/uimanager_bench.lua | 18 +++-- spec/unit/uimanager_spec.lua | 120 +++++++++++++++++--------------- 5 files changed, 169 insertions(+), 85 deletions(-) create mode 100644 spec/unit/taskqueue_bench.lua diff --git a/frontend/ui/uimanager.lua b/frontend/ui/uimanager.lua index 8b4763437..131921215 100644 --- a/frontend/ui/uimanager.lua +++ b/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 - lo = mid + 1 - else + if mid_time <= sched_time then hi = mid - 1 + else + lo = mid + 1 end end @@ -941,10 +941,9 @@ end --]] function UIManager:getNextTaskTime() - if self._task_queue[1] then - return self._task_queue[1].time - time:now() - else - return nil + local next_task = self._task_queue[#self._task_queue] + if next_task then + return next_task.time - time:now() 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... - local task = table.remove(self._task_queue, 1) + -- Remove the upcoming task, as it is due for execution... + 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 diff --git a/spec/unit/readerfooter_spec.lua b/spec/unit/readerfooter_spec.lua index f37ec6caa..ff635c625 100644 --- a/spec/unit/readerfooter_spec.lua +++ b/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 diff --git a/spec/unit/taskqueue_bench.lua b/spec/unit/taskqueue_bench.lua new file mode 100644 index 000000000..12b378537 --- /dev/null +++ b/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) diff --git a/spec/unit/uimanager_bench.lua b/spec/unit/uimanager_bench.lua index 05d0efc9f..39f15f30e 100644 --- a/spec/unit/uimanager_bench.lua +++ b/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) diff --git a/spec/unit/uimanager_spec.lua b/spec/unit/uimanager_spec.lua index d68cfbe68..b6735b788 100644 --- a/spec/unit/uimanager_spec.lua +++ b/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(future, UIManager._task_queue[1].time) - assert.are.same(future2, UIManager._task_queue[2].time) + assert.are.same(2, #UIManager._task_queue) + assert.are.same(future, 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" - -- NOTE: Can't use 10, as time.now, which is used internally, may or may not have moved, - -- depending on host's performance and clock granularity (especially if host is fast and/or COARSE is available). - UIManager:scheduleIn(11, 'foo') - assert.are.same('foo', UIManager._task_queue[4].action) + -- insert task in place of "10s", as it'll expire shortly afer "10s" + -- NOTE: Can't use this here right now, as time.now, which is used internally, + -- may or may not have moved, depending on host's performance and clock granularity + -- (especially if host is fast and/or COARSE is available). + -- 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) - -- "barba" is later than "nix" anyway + assert.are.same('nix', UIManager._task_queue[5].action) + -- "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, action = task_to_remove, args = {} }, -- this will be removed + { time = now - time.s(15), action = task_to_remove, args = {} }, -- this will be called } + UIManager:_checkTasks() assert.are.same(2, run_count) end)