From e7dd9737bd5deb573c9fef7b3ff2ead29b2cfe31 Mon Sep 17 00:00:00 2001 From: Jude Melton-Houghton Date: Sun, 16 Jan 2022 19:01:02 -0500 Subject: [PATCH] Reduce `minetest.after` time complexity and provide ordering guarantee --------- Co-authored-by: Lars Mueller --- builtin/common/after.lua | 190 +++++++++++++++++++++++----- builtin/common/tests/after_spec.lua | 113 +++++++++++++++++ builtin/game/features.lua | 1 + doc/client_lua_api.md | 4 + doc/lua_api.md | 5 + 5 files changed, 280 insertions(+), 33 deletions(-) create mode 100644 builtin/common/tests/after_spec.lua diff --git a/builtin/common/after.lua b/builtin/common/after.lua index bce262537..321e5ac9d 100644 --- a/builtin/common/after.lua +++ b/builtin/common/after.lua @@ -1,4 +1,116 @@ -local jobs = {} +-- This is an implementation of a job sheduling mechanism. It guarantees that +-- coexisting jobs will execute primarily in order of least expiry, and +-- secondarily in order of first registration. + +-- These functions implement an intrusive singly linked list of one or more +-- elements where the first element has a pointer to the last. The next pointer +-- is stored with key list_next. The pointer to the last is with key list_end. + +local function list_init(first) + first.list_end = first +end + +local function list_append(first, append) + first.list_end.list_next = append + first.list_end = append +end + +local function list_append_list(first, first_append) + first.list_end.list_next = first_append + first.list_end = first_append.list_end +end + +-- The jobs are stored in a map from expiration times to linked lists of jobs +-- as above. The expiration times are also stored in an array representing a +-- binary min heap, which is a particular arrangement of binary tree. A parent +-- at index i has children at indices i*2 and i*2+1. Out-of-bounds indices +-- represent nonexistent children. A parent is never greater than its children. +-- This structure means that, if there is at least one job, the next expiration +-- time is the first item in the array. + +-- Push element on a binary min-heap, +-- "bubbling up" the element by swapping with larger parents. +local function heap_push(heap, element) + local index = #heap + 1 + while index > 1 do + local parent_index = math.floor(index / 2) + local parent = heap[parent_index] + if element < parent then + heap[index] = parent + index = parent_index + else + break + end + end + heap[index] = element +end + +-- Pop smallest element from the heap, +-- "sinking down" the last leaf on the last layer of the heap +-- by swapping with the smaller child. +local function heap_pop(heap) + local removed_element = heap[1] + local length = #heap + local element = heap[length] + heap[length] = nil + length = length - 1 + if length > 0 then + local index = 1 + while true do + local old_index = index + local smaller_element = element + local left_index = index * 2 + local right_index = index * 2 + 1 + if left_index <= length then + local left_element = heap[left_index] + if left_element < smaller_element then + index = left_index + smaller_element = left_element + end + end + if right_index <= length then + if heap[right_index] < smaller_element then + index = right_index + end + end + if old_index ~= index then + heap[old_index] = heap[index] + else + break + end + end + heap[index] = element + end + return removed_element +end + +local job_map = {} +local expiries = {} + +-- Adds an individual job with the given expiry. +-- The worst-case complexity is O(log n), where n is the number of distinct +-- expiration times. +local function add_job(expiry, job) + local list = job_map[expiry] + if list then + list_append(list, job) + else + list_init(job) + job_map[expiry] = job + heap_push(expiries, expiry) + end +end + +-- Removes the next expiring jobs and returns the linked list of them. +-- The worst-case complexity is O(log n), where n is the number of distinct +-- expiration times. +local function remove_first_jobs() + local removed_expiry = heap_pop(expiries) + local removed = job_map[removed_expiry] + job_map[removed_expiry] = nil + return removed +end + local time = 0.0 local time_next = math.huge @@ -9,42 +121,54 @@ core.register_globalstep(function(dtime) return end - time_next = math.huge + -- Remove the expired jobs. + local expired = remove_first_jobs() - -- Iterate backwards so that we miss any new timers added by - -- a timer callback. - for i = #jobs, 1, -1 do - local job = jobs[i] - if time >= job.expire then - core.set_last_run_mod(job.mod_origin) - job.func(unpack(job.arg)) - local jobs_l = #jobs - jobs[i] = jobs[jobs_l] - jobs[jobs_l] = nil - elseif job.expire < time_next then - time_next = job.expire + -- Remove other expired jobs and append them to the list. + while true do + time_next = expiries[1] or math.huge + if time_next > time then + break end + list_append_list(expired, remove_first_jobs()) + end + + -- Run the callbacks afterward to prevent infinite loops with core.after(0, ...). + local last_expired = expired.list_end + while true do + core.set_last_run_mod(expired.mod_origin) + expired.func(unpack(expired.args, 1, expired.args.n)) + if expired == last_expired then + break + end + expired = expired.list_next end end) -function core.after(after, func, ...) - assert(tonumber(after) and type(func) == "function", - "Invalid minetest.after invocation") - local expire = time + after - local new_job = { - func = func, - expire = expire, - arg = {...}, - mod_origin = core.get_last_run_mod(), - } +local job_metatable = {__index = {}} - jobs[#jobs + 1] = new_job - time_next = math.min(time_next, expire) - - return { - cancel = function() - new_job.func = function() end - new_job.args = {} - end - } +local function dummy_func() end +function job_metatable.__index:cancel() + self.func = dummy_func + self.args = {n = 0} +end + +function core.after(after, func, ...) + assert(tonumber(after) and not core.is_nan(after) and type(func) == "function", + "Invalid minetest.after invocation") + + local new_job = { + mod_origin = core.get_last_run_mod(), + func = func, + args = { + n = select("#", ...), + ... + }, + } + + local expiry = time + after + add_job(expiry, new_job) + time_next = math.min(time_next, expiry) + + return setmetatable(new_job, job_metatable) end diff --git a/builtin/common/tests/after_spec.lua b/builtin/common/tests/after_spec.lua new file mode 100644 index 000000000..cca1871a5 --- /dev/null +++ b/builtin/common/tests/after_spec.lua @@ -0,0 +1,113 @@ +_G.core = {} +_G.vector = {metatable = {}} + +dofile("builtin/common/vector.lua") +dofile("builtin/common/misc_helpers.lua") + +function core.get_last_run_mod() return "*test*" end +function core.set_last_run_mod() end + +local do_step +function core.register_globalstep(func) + do_step = func +end + +dofile("builtin/common/after.lua") + +describe("after", function() + it("executes callbacks when expected", function() + local result = "" + core.after(0, function() + result = result .. "a" + end) + core.after(1, function() + result = result .. "b" + end) + core.after(1, function() + result = result .. "c" + end) + core.after(2, function() + result = result .. "d" + end) + local cancel = core.after(2, function() + result = result .. "e" + end) + do_step(0) + assert.same("a", result) + + do_step(1) + assert.same("abc", result) + + core.after(2, function() + result = result .. "f" + end) + core.after(1, function() + result = result .. "g" + end) + core.after(-1, function() + result = result .. "h" + end) + cancel:cancel() + do_step(1) + assert.same("abchdg", result) + + do_step(1) + assert.same("abchdgf", result) + end) + + it("defers jobs with delay 0", function() + local result = "" + core.after(0, function() + core.after(0, function() + result = result .. "b" + end) + result = result .. "a" + end) + do_step(1) + assert.same("a", result) + do_step(1) + assert.same("ab", result) + end) + + it("passes arguments", function() + core.after(0, function(...) + assert.same(0, select("#", ...)) + end) + core.after(0, function(...) + assert.same(4, select("#", ...)) + assert.same(1, (select(1, ...))) + assert.same(nil, (select(2, ...))) + assert.same("a", (select(3, ...))) + assert.same(nil, (select(4, ...))) + end, 1, nil, "a", nil) + do_step(0) + end) + + it("rejects invalid arguments", function() + assert.has.errors(function() core.after() end) + assert.has.errors(function() core.after(nil, nil) end) + assert.has.errors(function() core.after(0) end) + assert.has.errors(function() core.after(0, nil) end) + assert.has.errors(function() core.after(nil, function() end) end) + assert.has.errors(function() core.after(0 / 0, function() end) end) + end) + + -- Make sure that the underlying heap is working correctly + it("can be abused as a heapsort", function() + local t = {} + for i = 1, 1000 do + t[i] = math.random(100) + end + local sorted = table.copy(t) + table.sort(sorted) + local i = 0 + for _, v in ipairs(t) do + core.after(v, function() + i = i + 1 + assert.equal(v, sorted[i]) + end) + end + do_step(math.max(unpack(t))) + assert.equal(#t, i) + end) +end) diff --git a/builtin/game/features.lua b/builtin/game/features.lua index 9572596c6..67fe52289 100644 --- a/builtin/game/features.lua +++ b/builtin/game/features.lua @@ -31,6 +31,7 @@ core.features = { physics_overrides_v2 = true, hud_def_type_field = true, random_state_restore = true, + after_order_expiry_registration = true, } function core.has_feature(arg) diff --git a/doc/client_lua_api.md b/doc/client_lua_api.md index a6feb5c30..99f341de9 100644 --- a/doc/client_lua_api.md +++ b/doc/client_lua_api.md @@ -397,6 +397,10 @@ Call these functions only at load time! * `minetest.after(time, func, ...)` * Call the function `func` after `time` seconds, may be fractional * Optional: Variable number of arguments that are passed to `func` + * Jobs set for earlier times are executed earlier. If multiple jobs expire + at exactly the same time, then they expire in the order in which they were + registered. This basically just applies to jobs registered on the same + step with the exact same delay. * `minetest.get_us_time()` * Returns time with microsecond precision. May not return wall time. * `minetest.get_timeofday()` diff --git a/doc/lua_api.md b/doc/lua_api.md index 9adb29f71..af1980cee 100644 --- a/doc/lua_api.md +++ b/doc/lua_api.md @@ -5288,6 +5288,9 @@ Utilities -- PseudoRandom has get_state method -- PcgRandom has get_state and set_state methods (5.9.0) random_state_restore = true, + -- minetest.after guarantees that coexisting jobs are executed primarily + -- in order of expiry and secondarily in order of registration (5.9.0) + after_order_expiry_registration = true, } ``` @@ -6458,6 +6461,8 @@ Timing * `minetest.after(time, func, ...)`: returns job table to use as below. * Call the function `func` after `time` seconds, may be fractional * Optional: Variable number of arguments that are passed to `func` + * Jobs set for earlier times are executed earlier. If multiple jobs expire + at exactly the same time, then they are executed in registration order. * `job:cancel()` * Cancels the job function from being called