Reduce minetest.after time complexity and provide ordering guarantee

---------

Co-authored-by: Lars Mueller <appgurulars@gmx.de>

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

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)

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)

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()`

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