From a2a1f89926977fa9fb5a2c4e78fd321dc8bdd809 Mon Sep 17 00:00:00 2001 From: Ian Date: Wed, 29 Jul 2026 23:58:20 -0400 Subject: [PATCH 1/3] Prevent duplicate updates --- src/graph.luau | 169 +++++++++++++++++++++++++++++++++--------------- test/tests.luau | 108 +++++++++++++++---------------- todo.md | 1 - 3 files changed, 170 insertions(+), 108 deletions(-) diff --git a/src/graph.luau b/src/graph.luau index fd22489..f96bf4e 100644 --- a/src/graph.luau +++ b/src/graph.luau @@ -7,7 +7,7 @@ export type SourceNode = { export type Node = { cache: T, - effect: ((T) -> T) | false, + effect: ((T) -> T) | false, cleanups: { () -> () } | false, context: { [number]: unknown } | false, @@ -25,13 +25,11 @@ local function efn(err: string) local trace = debug.traceback(err, 2) if string.find(err, "^effect error stacktrace") then -- if effect error is nested - trace = string.gsub(" " .. trace, "\n", function() -- indent entire error - return "\n " - end) + trace = string.gsub(" " .. trace, "\n", "\n ") -- indent entire error end trace ..= "\nsource update stacktrace:" -return trace + return trace end local function ycall(fn: (T) -> U, arg: T): (boolean, string|U) @@ -41,7 +39,7 @@ local function ycall(fn: (T) -> U, arg: T): (boolean, string|U) local resume_ok, run_ok, result = coroutine.resume(thread, fn, efn, arg) assert(resume_ok) - + if coroutine.status(thread) ~= "dead" then return false, debug.traceback(thread, "attempt to yield in reactive scope") end @@ -125,7 +123,7 @@ local function destroy(node: Node) flush_cleanups(node) unparent(node) - + if node.owner then find_and_swap_pop(node.owner.owned :: { Node }, node) node.owner = false @@ -144,9 +142,19 @@ local function destroy_owned(node: Node) end end -local update_queue = { n = 0 } :: { n: number, [number]: Node } +local update_queue_n = 0 +local in_queue = {} +local update_queue = {} :: { [number]: Node } +local node_deps_left = {} -- any node that goes into the update queue needs to store how many dependencies it's waiting on + +local function reset_queue() + update_queue_n = 0 + table.clear(in_queue) + table.clear(update_queue) + table.clear(node_deps_left) +end -local function evaluate_node(node: Node) +local function evaluate_node(node: Node, continue_on_error: boolean) -- if continue_on_error, the calling function wishes to continue even if the update fails (without using pcall) if flags.strict then if table.find(scopes, node) then error("a scope, that should rerun due to the update of a source, is already active", 0) @@ -159,15 +167,19 @@ local function evaluate_node(node: Node) flush_cleanups(node) destroy_owned(node) - + push_scope(node) local ok, new_value = ycall(node.effect :: (T) -> T, cur_value) pop_scope() - + if not ok then - table.clear(update_queue) - update_queue.n = 0 - error(`effect error stacktrace\n{new_value :: string}`, 0) + local msg = debug.traceback(`effect error: {new_value}`, 2) + if continue_on_error then + task.spawn(error, msg, 0) + return false + else + error(msg, 2) + end end node.cache = new_value :: T @@ -181,72 +193,123 @@ local function evaluate_node(node: Node) destroy_owned(node) push_scope(node) - local ok, new_value = pcall(node.effect :: (T) -> T, node.cache) + local ok, new_value = pcall(node.effect :: (T) -> T, cur_value) pop_scope() if not ok then - table.clear(update_queue) - update_queue.n = 0 - error(`effect error:\n{new_value}\n`, 0) + local msg = debug.traceback(`effect error: {new_value}`, 2) + if continue_on_error then + task.spawn(error, msg, 0) + return false + else + error(msg, 2) + end end - + node.cache = new_value return cur_value ~= new_value end end +local function add_dependency_counts(node) + for i = 1, #node do + local child = node[i] + if not child.owner then continue end -- we won't be updating this anyway + local deps_left = node_deps_left[child] + if deps_left then + node_deps_left[child] = deps_left + 1 + else + node_deps_left[child] = 1 + add_dependency_counts(child) -- only recurse if we haven't seen this node before + end + end +end +local function remove_dependency_counts(node) -- to be called when updating a node if its value remains unchanged + for i = 1, #node do + local child = node[i] + if not node.owner then continue end -- we won't be updating this anyway + node_deps_left[child] -= 1 + remove_dependency_counts(child) + end +end + local function queue_children_for_update(node: SourceNode) - local i = update_queue.n - while node[1] do - i += 1 - update_queue[i] = node[1] - unparent(node[1]) + for i = 1, #node do + local child = node[i] + node_deps_left[child] -= 1 + if not in_queue[child] then + in_queue[child] = true + update_queue_n += 1 + update_queue[update_queue_n] = child + end end - update_queue.n = i end local function get_update_queue_length() - return update_queue.n + return update_queue_n end local function flush_update_queue(from: number) - local i = from + 1 - while i <= update_queue.n do - local node = update_queue[i] - --assert(node.effect) + while true do + local i = from + if i >= update_queue_n then return end -- nothing more to do + local changed = false + local moveTo1 = from -- 1 less than where to store an update if we can't process it yet + while i < update_queue_n do + i += 1 + local node = update_queue[i] + -- assert(node.effect) + if node.owner then + local deps_left = node_deps_left[node] + if deps_left > 0 then -- waiting on more dependencies + moveTo1 += 1 + if i ~= moveTo1 then + update_queue[moveTo1] = node + update_queue[i] = false :: any + end + continue + end + changed = true + -- Reset in_queue and node_deps_left before evaluation in case the node's depedencies are changed during evaluation (which usually implies an infinite loop, but we have explicit errors for this case) + in_queue[node] = nil + node_deps_left[node] = nil + unparent(node) + if evaluate_node(node, true) then + queue_children_for_update(node) + else + remove_dependency_counts(node) + end + else -- otherwise node was cleaned up + in_queue[node] = nil + node_deps_left[node] = nil + end - if node.owner and evaluate_node(node) then - queue_children_for_update(node) + update_queue[i] = false :: any + end + if moveTo1 == 0 then -- no pending updates + reset_queue() + return + else + update_queue_n = moveTo1 + if not changed then -- Can occur in recursive updates (where 'from' is > 0) + if from == 0 then + warn("Some nodes failed to update", update_queue, node_deps_left, debug.traceback()) + reset_queue() + end + return + end end - - update_queue[i] = false :: any - i += 1 end - - update_queue.n = from end local function update_descendants(root: SourceNode) - local n0 = update_queue.n + add_dependency_counts(root) + local n0 = update_queue_n queue_children_for_update(root) if flags.batch then return end - local i = n0 + 1 - while i <= update_queue.n do - local node = update_queue[i] - --assert(node.effect) - - -- check if node is still owned in case destroyed after queued - if node.owner and evaluate_node(node) then - queue_children_for_update(node) - end - - update_queue[i] = false :: any -- false instead of nil to avoid sparse - i += 1 - end - - update_queue.n = n0 + flush_update_queue(n0) end local function push_scope_as_child_of(node: SourceNode) diff --git a/test/tests.luau b/test/tests.luau index af6a7f4..8b2b574 100644 --- a/test/tests.luau +++ b/test/tests.luau @@ -116,7 +116,7 @@ TEST("graph", function() effect(c.cache) pop_scope() - + CHECK(count == 1) update_descendants(a) CHECK(count == 2) @@ -188,7 +188,7 @@ TEST("graph", function() end) end - do push_scope(root) + do push_scope(root) clean "root" items_updated = node(root) push_scope_as_child_of(items_updated) -- should not @@ -216,9 +216,9 @@ TEST("graph", function() pop_scope() end pop_scope() end pop_scope() end - pop_scope() end + pop_scope() end pop_scope() end - + -- verify graph @@ -226,7 +226,7 @@ TEST("graph", function() local c = get_children(items_updated) CHECK(#c == 0) end - + do local c = get_children(root) CHECK(#c == 0) @@ -298,7 +298,7 @@ TEST("graph", function() depth=1 _, _ <- attempt to update_descendants nothing - ^ + ^ ]] @@ -466,7 +466,7 @@ TEST("derive()", wrap_root(function() is_even() count += 1 end) - + num(1) -- odd CHECK(count == 2) num(2) -- even @@ -559,7 +559,7 @@ TEST("derive()", wrap_root(function() -- end) -- destroy() - + -- CHECK(ran[1] == 1) -- CHECK(ran[2] == 1) -- CHECK(cleaned[1] == 1) @@ -567,7 +567,7 @@ TEST("derive()", wrap_root(function() -- end do CASE "garbage collection" - -- check that `b` does not allow gc of `a` + -- check that `b` does not allow gc of `a` local a = source(1) local _b = derive(function() @@ -606,7 +606,7 @@ TEST("effect()", wrap_root(function() local num = source(0) local text = derive(function() return tostring(num()) end) - + local count = 0 effect(function() text() @@ -660,7 +660,7 @@ TEST("cleanup()", wrap_root(function() src() effected += 1 cleanup(function() - cleaned += 1 + cleaned += 1 end) end) @@ -677,7 +677,7 @@ TEST("cleanup()", wrap_root(function() local src = source(1) local queue = {} - + effect(function() src() cleanup(function() table.insert(queue, 1) end) @@ -772,7 +772,7 @@ TEST("create()", wrap_root(function() vide.defer_nested_properties = false local t = {} - + create "TextLabel" { { { function() table.insert(t, 1) end } :: any, @@ -891,7 +891,7 @@ TEST("create()", wrap_root(function() do CASE "bind children" local children = source() - local a, b, c = + local a, b, c = create "TextLabel" { Name = "A" }, create "TextLabel" { Name = "B" }, create "TextLabel" { Name = "C" } @@ -899,14 +899,14 @@ TEST("create()", wrap_root(function() local frame = create "Frame" { children } - + children { a, b } CHECK(frame:FindFirstChild "A") CHECK(frame:FindFirstChild "B") -- check that b is removed and c is added while a remains untouched - + children { a, c } CHECK(frame:FindFirstChild "A") @@ -949,7 +949,7 @@ TEST("create()", wrap_root(function() local set_test_to_true = action(function(self) (self :: any).test = true end) local f2 - + local to_apply = { { a = 1 }, set_test_to_true, @@ -978,13 +978,13 @@ TEST("create()", wrap_root(function() local c = create "Frame" { Name = "c" } local d = create "Frame" { Name = "d" } local e = create "Frame" { Name = "e" } - + local children = source { a, { b, c, { d } }, { { e } } } - + local obj = create "Frame" { children } @@ -1125,7 +1125,7 @@ TEST("show()", wrap_root(function() show(input :: () -> number?, function(value: () -> number) effect(function() local v = value() - + count += 1 CHECK(v == count) @@ -1166,7 +1166,7 @@ TEST("show()", wrap_root(function() show(weapon, function(weapon: () -> Weapon) local enchant = function() return weapon().enchant end - + show(enchant, function(enchant: () -> string) effect(function() local e = enchant() @@ -1295,7 +1295,7 @@ TEST("show()", wrap_root(function() CHECK(value_upval() == true) CHECK(present_upval() == false) CHECK(not cleaned) - + step(0.5) CHECK(output() == obj) @@ -1342,7 +1342,7 @@ TEST("show()", wrap_root(function() CHECK(value_upval() == true) CHECK(present_upval() == false) CHECK(not cleaned) - + step(0.5) CHECK(output() == obj) @@ -1432,15 +1432,15 @@ TEST("switch()", wrap_root(function() CHECK(count == 1) CHECK(output() == 1) - + input(false) CHECK(output() == 0) CHECK(count == 2) - + input(false) CHECK(output() == 0) CHECK(count == 2) - + input(NIL) CHECK(output() == nil) end @@ -1477,12 +1477,12 @@ TEST("switch()", wrap_root(function() cleanup(function() owner_count += 1 end) local output = switch(input) { - [true] = function() + [true] = function() cleanup(function() switch1_count += 1 end) return 1 end, - [false] = function() + [false] = function() cleanup(function() switch0_count += 1 end) return 0 end @@ -1546,7 +1546,7 @@ TEST("indexes()", wrap_root(function() local input = source { 1, 2, 3 } local output = indexes(input, function(v, k) - return tostring(v()) + return tostring(v()) end) CHECK("" .. input()[1] == output()[1]) @@ -1619,7 +1619,7 @@ TEST("indexes()", wrap_root(function() input = NIL gc() - CHECK(wref[1]) + CHECK(wref[1]) end do -- check that `input` allows gc of `output` @@ -1645,7 +1645,7 @@ TEST("indexes()", wrap_root(function() local input = source { 1, 2, 3 } local count = table.create(3, 0) - + local output = indexes(input, function(v, i) cleanup(function() count[i] += 1 @@ -1691,12 +1691,12 @@ TEST("indexes()", wrap_root(function() indexes(items, function(item) effect(function() item() - updated[1] += 1 + updated[1] += 1 end) effect(function() item() - updated[2] += 1 + updated[2] += 1 end) return {} @@ -1739,7 +1739,7 @@ TEST("indexes()", wrap_root(function() local input = source {} local cleaned_counts = {} :: Map - + local output = indexes(input, function(v, i, present) cleanup(function() cleaned_counts[i] = (cleaned_counts[i] or 0) + 1 @@ -1816,7 +1816,7 @@ TEST("values()", wrap_root(function() local input = source { 1, 2, 3 } local output = values(input, function(v, k) - return tostring(v) + return tostring(v) end) CHECK("" .. input()[1] == output()[1]) @@ -1894,7 +1894,7 @@ TEST("values()", wrap_root(function() local input = source { 1, 2, 3 } local count = table.create(3, 0) - + local output = values(input, function(v, i) cleanup(function() count[i()] += 1 @@ -1935,7 +1935,7 @@ TEST("values()", wrap_root(function() local input = source {} local cleaned_counts = {} :: Map - + local output = values(input, function(v, i, present) cleanup(function() cleaned_counts[v] = (cleaned_counts[v] or 0) + 1 @@ -2056,10 +2056,10 @@ TEST("spring()", wrap_root(function() do -- `input` should allow gc of `output` local input = source(10) local output = spring(input) - + local wref = weak { output } output = NIL - + gc() CHECK(not wref[1]) end @@ -2073,7 +2073,7 @@ TEST("spring()", wrap_root(function() -- local output_node = unpack(capture(output)) -- wref = weak { output_node, data } -- end - + -- gc() -- CHECK(not wref[1]) -- CHECK(not wref[2]) @@ -2208,11 +2208,11 @@ TEST("untrack()", wrap_root(function() return untrack(function() return derive(function() inner_count += 1 - + cleanup(function() cleaned_count += 1 end) - + return tostring(input()) end) end) @@ -2435,7 +2435,7 @@ TEST("batch()", wrap_root(function() b(a()) c(a()) end) - + batch(function() a(1) end) @@ -2491,10 +2491,10 @@ TEST("batch()", wrap_root(function() do CASE "recursive queue flush diamond A,B,C,D,E" --[[ where b and c batches d - + a > b > e > c > d > - + ]] local a = source(0) @@ -2551,7 +2551,7 @@ TEST("batch()", wrap_root(function() --[[ a > b > d > E > G - > c ^ > F + > c ^ > F ]] @@ -2560,7 +2560,7 @@ TEST("batch()", wrap_root(function() local b = source(0) local c = source(0) local d = source(0) - + local e = source(0) local f = source(0) local g = source(0) @@ -2668,7 +2668,7 @@ TEST("context()", function() local ok = pcall(function() ctx(1, function() end) end) - + CHECK(not ok) end @@ -2847,14 +2847,14 @@ TEST("graph edge cases", wrap_root(function() CHECK(count.b == 2) CHECK(count.c == 2) CHECK(count.d == 2) - CHECK(count.e == 3) -- todo: redundant re-eval + CHECK(count.e == 2) CHECK(e() == 4) a(3) CHECK(count.b == 2) CHECK(count.c == 3) CHECK(count.d == 3) - CHECK(count.e == 4) + CHECK(count.e == 3) CHECK(e() == 12) end @@ -2871,7 +2871,7 @@ TEST("graph edge cases", wrap_root(function() end do CASE "do not destroy children" - local parent = source(0) + local parent = source(0) local _, destroy, @@ -2899,10 +2899,10 @@ TEST("graph edge cases", wrap_root(function() CHECK(count == 3) destroy() - + update_parent_to_destroy(2) CHECK(count == 3) - + parent(parent() + 1) CHECK(count == 4) end @@ -2931,7 +2931,7 @@ TEST("graph edge cases", wrap_root(function() local _, destroy_child, _child_B = nil, function() end, nil local count_A = 0 - + -- child_A effect(function() count_A += 1 diff --git a/todo.md b/todo.md index 6f254bd..499fbb8 100644 --- a/todo.md +++ b/todo.md @@ -1,4 +1,3 @@ # todo - improve error traces -- prevent redundant re-eval of nodes in a complex diamond graph From ec3584e7c25efa023ba906ee0a8ee9e5a3be7578 Mon Sep 17 00:00:00 2001 From: Ian Date: Mon, 3 Aug 2026 15:32:25 -0400 Subject: [PATCH 2/3] Fix --- src/graph.luau | 20 +++++++++++++++++--- todo.md | 1 + 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/graph.luau b/src/graph.luau index f96bf4e..38d2bfc 100644 --- a/src/graph.luau +++ b/src/graph.luau @@ -228,15 +228,29 @@ local function remove_dependency_counts(node) -- to be called when updating a no for i = 1, #node do local child = node[i] if not node.owner then continue end -- we won't be updating this anyway - node_deps_left[child] -= 1 - remove_dependency_counts(child) + local deps_left = node_deps_left[child] + if deps_left > 1 then + node_deps_left[child] = deps_left - 1 + else + node_deps_left[child] = 0 + -- only recurse if we're removing the last dependency *and* if we aren't planning on updating it + if not in_queue[child] then + remove_dependency_counts(child) + end + end end end local function queue_children_for_update(node: SourceNode) for i = 1, #node do local child = node[i] - node_deps_left[child] -= 1 + local deps_left = node_deps_left[child] + if deps_left then + node_deps_left[child] = deps_left - 1 + else + -- This can happen if child adds parent as a new dependency after the initial update + node_deps_left[child] = 0 + end if not in_queue[child] then in_queue[child] = true update_queue_n += 1 diff --git a/todo.md b/todo.md index 499fbb8..2142d7b 100644 --- a/todo.md +++ b/todo.md @@ -1,3 +1,4 @@ # todo - improve error traces +- prevent redundant re-eval of nodes in complex graphs after a node adds a dependency mid-update From ff020bacb8634d805502d17fc857ca376eb2359d Mon Sep 17 00:00:00 2001 From: Ian Date: Mon, 3 Aug 2026 15:38:31 -0400 Subject: [PATCH 3/3] Add duplicate update test --- src/graph.luau | 6 ++-- test/tests.luau | 96 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 3 deletions(-) diff --git a/src/graph.luau b/src/graph.luau index 38d2bfc..f637906 100644 --- a/src/graph.luau +++ b/src/graph.luau @@ -175,7 +175,7 @@ local function evaluate_node(node: Node, continue_on_error: boolean) -- if if not ok then local msg = debug.traceback(`effect error: {new_value}`, 2) if continue_on_error then - task.spawn(error, msg, 0) + print(msg) return false else error(msg, 2) @@ -199,7 +199,7 @@ local function evaluate_node(node: Node, continue_on_error: boolean) -- if if not ok then local msg = debug.traceback(`effect error: {new_value}`, 2) if continue_on_error then - task.spawn(error, msg, 0) + print(msg) return false else error(msg, 2) @@ -307,7 +307,7 @@ local function flush_update_queue(from: number) update_queue_n = moveTo1 if not changed then -- Can occur in recursive updates (where 'from' is > 0) if from == 0 then - warn("Some nodes failed to update", update_queue, node_deps_left, debug.traceback()) + print("Some nodes failed to update", update_queue, node_deps_left, debug.traceback()) reset_queue() end return diff --git a/test/tests.luau b/test/tests.luau index 8b2b574..e24bfb1 100644 --- a/test/tests.luau +++ b/test/tests.luau @@ -319,6 +319,102 @@ TEST("graph", function() end end) +TEST("duplicate update", function() + local cases = { + { + name = "b", + err = "b", + e1 = {1, 2, 2, 2, 2}, + }, + { + name = "c", + err = "c", + e1 = {2, 1, 1, 2, 2}, + }, + { + name = "c2", + err = "c2", + e1 = {2, 2, 1, 2, 2}, + }, + { + name = "d", + err = "d", + e1 = {2, 2, 2, 1, 2}, + }, + { + name = "e", + err = "e", + e1 = {2, 2, 2, 2, 1}, + e2 = {3, 3, 3, 3, 3}, -- e has a dependency on just 'a' after the first time, so it'll get updated 2x (only way to prevent this extra update is to yield, which isn't currently supported) + }, + { + name = "no error", + err = "", + e1 = {2, 2, 2, 2, 2}, + } + } + for _, case in cases do + CASE(case.name) + local destroy = root(function() + local e1, e2, err = case.e1, case.e2, case.err + if not e2 then + e2 = {} + for i, v in e1 do + e2[i] = v + 1 + end + end + local a = source(0) + local nb = 0 + local b = derive(function() + if err == "b" and a() == 1 then error("user error") end + nb += 1 + return a() + 1 + end) + local nc = 0 + local c = derive(function() + if err == "c" and a() == 1 then error("user error") end + nc += 1 + return a() * 2 + end) + local nc2 = 0 + local c2 = derive(function() + if err == "c2" and c() == 2 then error("user error") end + nc2 += 1 + return c() + end) + local nd = 0 + local d = derive(function() + if err == "d" and b() == 2 then error("user error") end + nd += 1 + return b() * 100 + c2() + end) + local ne = 0 + effect(function() + if err == "e" and a() == 1 then error("user error") end + ne += 1 + a(); b(); c(); c2(); d() + end) + local length = graph.get_update_queue_length() + + a(1) + CHECK(nb == e1[1]) + CHECK(nc == e1[2]) + CHECK(nc2 == e1[3]) + CHECK(nd == e1[4]) + CHECK(ne == e1[5]) + CHECK(graph.get_update_queue_length() == length) + + a(2) + CHECK(nb == e2[1]) + CHECK(nc == e2[2]) + CHECK(nc2 == e2[3]) + CHECK(nd == e2[4]) + CHECK(ne == e2[5]) + end) + destroy() + end +end) + TEST("mount()", function() local screen = create "ScreenGui" {}