diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ae63b04..a620e87d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ Making a new release? Simply add the new header with the version and date undern ## Unreleased +* Replace greedy matching with optimal Hungarian (Kuhn-Munkres) algorithm in forward sync, syncback, and plugin reconciler for correct instance pairing when multiple same-named instances exist in ambiguous groups * Optimize syncback performance for large projects (~42% faster): skip hash computation in clean mode, reuse prefetch walkdir for orphan detection, rewrite collect_all_paths as O(n) top-down BFS, add file-size-first comparison in write phase, scope git index queries to project directory ([#10]) [#10]: https://github.com/UserGeneratedLLC/rojo/pull/10 diff --git a/Cargo.toml b/Cargo.toml index ff69fdee..6ce1a57a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -113,6 +113,7 @@ dirs = "6" pathdiff = "0.2.3" blake3 = "1.8.3" +rand = "0.10.0" sha1 = "0.10" indexmap = { version = "2.13.0", features = ["serde"] } @@ -144,4 +145,3 @@ regex = "1" serde_yaml = "0.9" tempfile = "3" walkdir = "2.5.0" -rand = "0.10.0" diff --git a/TODO.txt b/TODO.txt index dae15064..6d5e361a 100644 --- a/TODO.txt +++ b/TODO.txt @@ -90,22 +90,9 @@ in all 3 matchers, simply randomize the order of the children - this will break can we allow the terminal to stay so i can like run some git commands myself without having to make a whole new terminal? i also want to just cleanup terminals / use one terminal for syncback and serve... -[multi place clone] -make clone support multi-placeids like we did with defend-67 in D:\UserGenerated\place-for-defend-67 (i did this manually) -see specifically game/ and game.project.json5, lobby/ and lobby.project.json5. -atlas clone PLACEID1 PLACEID2 ..... -This is extremely helpful for games that have diverged places within the same game, e.g. a lobby and an actual game place -we will probably have to start dynamically generating the x.project.json5 rather than using a template file now though, which is fine.. - [syncback] optimize syncback like we did serve. [sourcemap / luau-lsp] we need to fix "luau-lsp.sourcemap.generatorCommand": "atlas sourcemap default.project.json5 --output sourcemap.json --include-non-scripts --watch", and make luau-lsp actually use the 'atlas' cli... - -[two way sync] -so the plugin isnt respecting two-way sync for glob ignore -basically if a new script is on studio that isn't in atlas but even though it's glob ignored, it still tries to two-way sync it over and say there has been a change - -[hit up coelho] \ No newline at end of file diff --git a/plugin/src/Hungarian.lua b/plugin/src/Hungarian.lua new file mode 100644 index 00000000..d166d74c --- /dev/null +++ b/plugin/src/Hungarian.lua @@ -0,0 +1,103 @@ +--!strict +--!native +--!optimize 2 +--[[ + Minimum-cost bipartite matching via the Hungarian (Kuhn-Munkres) algorithm. + + Given a 1-indexed cost matrix cost[i][j] for `rows` row-agents and `cols` + column-agents, returns an array of {row, col} pairs representing the + optimal assignment that minimizes total cost. Handles rectangular matrices + by padding to square with `padCost`. + + Complexity: O(N^3) where N = max(rows, cols). +]] + +local function minCostAssignment(cost: { { number } }, rows: number, cols: number, padCost: number): { { number } } + if rows == 0 or cols == 0 then + return {} + end + + local n = math.max(rows, cols) + local S = n + 1 + local INF = math.huge + + local c: { { number } } = table.create(n) + for i = 1, n do + local row = table.create(n, padCost) + for j = 1, n do + if i <= rows and j <= cols then + row[j] = cost[i][j] + end + end + c[i] = row + end + + local u = table.create(S, 0) + local v = table.create(S, 0) + local p = table.create(S, 0) + local way = table.create(S, 0) + + for i = 1, n do + p[S] = i + local j0 = S + local minV = table.create(S, INF) + local used = table.create(S, false) + + while true do + used[j0] = true + local i0 = p[j0] + local delta = INF + local j1 = S + + for j = 1, n do + if used[j] then + continue + end + local cur = c[i0][j] - u[i0] - v[j] + if cur < minV[j] then + minV[j] = cur + way[j] = j0 + end + if minV[j] < delta then + delta = minV[j] + j1 = j + end + end + + u[p[S]] += delta + v[S] -= delta + for j = 1, n do + if used[j] then + u[p[j]] += delta + v[j] -= delta + else + minV[j] -= delta + end + end + + j0 = j1 + if p[j0] == 0 then + break + end + end + + while true do + local j1 = way[j0] + p[j0] = p[j1] + j0 = j1 + if j0 == S then + break + end + end + end + + local result: { { number } } = {} + for j = 1, n do + if p[j] ~= 0 and p[j] <= rows and j <= cols then + table.insert(result, { p[j], j }) + end + end + return result +end + +return minCostAssignment diff --git a/plugin/src/Reconciler/matching.lua b/plugin/src/Reconciler/matching.lua index 12e3de05..f6fd1817 100644 --- a/plugin/src/Reconciler/matching.lua +++ b/plugin/src/Reconciler/matching.lua @@ -23,6 +23,7 @@ local Packages = script.Parent.Parent.Parent.Packages local RbxDom = require(Packages.RbxDom) local trueEquals = require(script.Parent.trueEquals) +local minCostAssignment = require(script.Parent.Parent.Hungarian) local UNMATCHED_PENALTY = 10000 local MAX_SCORING_DEPTH = 3 @@ -89,13 +90,6 @@ type ClassComparisonKeys = { defaults: { [string]: any }, } -type ScoredPair = { - vi: number, - si: number, - cost: number, - idx: number, -} - -- ================================================================ -- Session constructor -- ================================================================ @@ -527,74 +521,66 @@ matchChildren = function( sCaches[si] = cacheStudio(studioChildren[si], classKeys, extraPropNamesArray, refPropNamesArray) end - local scoredPairs: { ScoredPair } = {} - local pairIdx = 0 - local bestSoFar = math.huge + -- Build cost matrix for the Hungarian algorithm. + local m = #vIndices + local n = #sIndices + local costMatrix: { { number } } = table.create(m) - for _, vi in ipairs(vIndices) do + for ri, vi in ipairs(vIndices) do local vCache = vCaches[vi] - if not vCache then - continue - end + local row = table.create(n, UNMATCHED_PENALTY) + if vCache then + for ci, si in ipairs(sIndices) do + local sCache = sCaches[si] + if not sCache then + continue + end - for _, si in ipairs(sIndices) do - local sCache = sCaches[si] - if not sCache then - continue - end + local cost = countOwnDiffs(vCache, sCache, classKeys) - pairIdx += 1 - local cost = countOwnDiffs(vCache, sCache, classKeys) + if depth < MAX_SCORING_DEPTH then + local validVChildren = vCache.validChildren + local studioKids = sCache.children - if cost < bestSoFar and depth < MAX_SCORING_DEPTH then - local validVChildren = vCache.validChildren - local studioKids = sCache.children - - if #validVChildren > 0 or #studioKids > 0 then - if #validVChildren == 0 then - cost += #studioKids * UNMATCHED_PENALTY - elseif #studioKids == 0 then - cost += #validVChildren * UNMATCHED_PENALTY - else - local childResult = matchChildren( - session, - validVChildren, - studioKids, - virtualInstances, - virtualChildren[vi], - sCache.instance, - depth + 1 - ) - cost += childResult.totalCost + if #validVChildren > 0 or #studioKids > 0 then + if #validVChildren == 0 then + cost += #studioKids * UNMATCHED_PENALTY + elseif #studioKids == 0 then + cost += #validVChildren * UNMATCHED_PENALTY + else + local childResult = matchChildren( + session, + validVChildren, + studioKids, + virtualInstances, + virtualChildren[vi], + sCache.instance, + depth + 1 + ) + cost += childResult.totalCost + end end end - end - table.insert(scoredPairs, { vi = vi, si = si, cost = cost, idx = pairIdx }) - if cost < bestSoFar then - bestSoFar = cost + row[ci] = cost end end + costMatrix[ri] = row end - table.sort(scoredPairs, function(a: ScoredPair, b: ScoredPair): boolean - if a.cost ~= b.cost then - return a.cost < b.cost - end - return a.idx < b.idx - end) - - for _, pair in ipairs(scoredPairs) do - if matchedV[pair.vi] or matchedS[pair.si] then - continue - end + -- Optimal assignment via the Hungarian algorithm. + local assignment = minCostAssignment(costMatrix, m, n, UNMATCHED_PENALTY * 2) + for _, pair in ipairs(assignment) do + local vi = vIndices[pair[1]] + local si = sIndices[pair[2]] + local cost = costMatrix[pair[1]][pair[2]] table.insert(matched, { - virtualId = virtualChildren[pair.vi], - studioInstance = studioChildren[pair.si], + virtualId = virtualChildren[vi], + studioInstance = studioChildren[si], }) - table.insert(matchedCosts, pair.cost) - matchedV[pair.vi] = true - matchedS[pair.si] = true + table.insert(matchedCosts, cost) + matchedV[vi] = true + matchedS[si] = true end end diff --git a/src/hungarian.rs b/src/hungarian.rs new file mode 100644 index 00000000..fc0b29f5 --- /dev/null +++ b/src/hungarian.rs @@ -0,0 +1,103 @@ +//! Minimum-cost bipartite matching via the Hungarian (Kuhn-Munkres) algorithm. +//! +//! Given a cost matrix `cost[i][j]` for `rows` row-agents and `cols` +//! column-agents, returns a Vec of `(row, col)` pairs representing the +//! optimal assignment that minimizes total cost. Handles rectangular +//! matrices by padding to square with `pad_cost`. +//! +//! Complexity: O(N^3) where N = max(rows, cols). + +/// Compute the minimum-cost assignment for a (possibly rectangular) cost matrix. +/// +/// `pad_cost` fills dummy entries when the matrix is padded to square. +/// It should be larger than any real cost in the matrix so that dummy +/// assignments are never preferred over real ones. +pub fn min_cost_assignment( + cost: &[Vec], + rows: usize, + cols: usize, + pad_cost: u32, +) -> Vec<(usize, usize)> { + if rows == 0 || cols == 0 { + return Vec::new(); + } + + let n = rows.max(cols); + let big = pad_cost as i64; + + let mut c = vec![vec![0i64; n]; n]; + for i in 0..n { + for j in 0..n { + c[i][j] = if i < rows && j < cols { + cost[i][j] as i64 + } else { + big + }; + } + } + + let mut u = vec![0i64; n + 1]; + let mut v = vec![0i64; n + 1]; + let mut p = vec![0usize; n + 1]; + let mut way = vec![0usize; n + 1]; + + for i in 1..=n { + p[0] = i; + let mut j0 = 0usize; + let mut min_v = vec![i64::MAX; n + 1]; + let mut used = vec![false; n + 1]; + + loop { + used[j0] = true; + let i0 = p[j0]; + let mut delta = i64::MAX; + let mut j1 = 0usize; + + for j in 1..=n { + if used[j] { + continue; + } + let cur = c[i0 - 1][j - 1] - u[i0] - v[j]; + if cur < min_v[j] { + min_v[j] = cur; + way[j] = j0; + } + if min_v[j] < delta { + delta = min_v[j]; + j1 = j; + } + } + + for j in 0..=n { + if used[j] { + u[p[j]] += delta; + v[j] -= delta; + } else { + min_v[j] -= delta; + } + } + + j0 = j1; + if p[j0] == 0 { + break; + } + } + + loop { + let j1 = way[j0]; + p[j0] = p[j1]; + j0 = j1; + if j0 == 0 { + break; + } + } + } + + let mut result = Vec::with_capacity(rows.min(cols)); + for j in 1..=n { + if p[j] != 0 && p[j] <= rows && j <= cols { + result.push((p[j] - 1, j - 1)); + } + } + result +} diff --git a/src/lib.rs b/src/lib.rs index 1269c257..f1a17326 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -10,6 +10,7 @@ mod tree_view; mod change_processor; pub mod git; mod glob; +pub mod hungarian; mod json; pub mod logging; mod lua_ast; diff --git a/src/snapshot/matching.rs b/src/snapshot/matching.rs index 4d81b232..d3302f91 100644 --- a/src/snapshot/matching.rs +++ b/src/snapshot/matching.rs @@ -5,7 +5,8 @@ //! //! Algorithm per parent: //! 1. Group by (Name, ClassName) -- 1:1 groups instant-match -//! 2. Ambiguous groups: recursive change-count scoring + greedy assignment +//! 2. Ambiguous groups: recursive change-count scoring + optimal assignment +//! via the Hungarian algorithm //! //! The change count = how many things the reconciler would need to touch //! to turn instance A into instance B, including the entire subtree. @@ -125,14 +126,13 @@ pub fn match_forward( } // ================================================================ - // Ambiguous groups: change-count scoring + greedy assignment + // Ambiguous groups: change-count scoring + optimal assignment // ================================================================ for (key, snap_indices) in &snap_by_key { let Some(tree_indices) = tree_by_key.get(key) else { continue; }; - // Collect unmatched indices in this group let avail_snap: Vec = snap_indices .iter() .filter(|&&si| !snap_matched[si]) @@ -147,7 +147,6 @@ pub fn match_forward( if avail_snap.is_empty() || avail_tree.is_empty() { continue; } - // 1:1 already handled above; skip if not truly ambiguous if avail_snap.len() <= 1 && avail_tree.len() <= 1 { if avail_snap.len() == 1 && avail_tree.len() == 1 { let si = avail_snap[0]; @@ -159,33 +158,31 @@ pub fn match_forward( continue; } - // Score all (A, B) pairs using recursive change-count scoring - let mut pairs: Vec<(u32, usize, usize)> = - Vec::with_capacity(avail_snap.len() * avail_tree.len()); - let mut best_so_far = u32::MAX; + let m = avail_snap.len(); + let n = avail_tree.len(); + let pad = UNMATCHED_PENALTY.saturating_mul(2); + let mut cost_matrix = Vec::with_capacity(m); for &si in &avail_snap { let snap = match &snap_available[si] { Some(s) => s, - None => continue, + None => { + cost_matrix.push(vec![pad; n]); + continue; + } }; + let mut row = Vec::with_capacity(n); for &ti in &avail_tree { let cost = - compute_change_count(snap, tree_children[ti], tree, best_so_far, 0, session); - pairs.push((cost, si, ti)); - if cost < best_so_far { - best_so_far = cost; - } + compute_change_count(snap, tree_children[ti], tree, u32::MAX, 0, session); + row.push(cost); } + cost_matrix.push(row); } - // Stable sort by cost ascending (slice::sort_by is stable in Rust) - pairs.sort_by_key(|&(cost, _, _)| cost); - - // Greedy assign - for &(_, si, ti) in &pairs { - if snap_matched[si] || !tree_available[ti] { - continue; - } + let assignment = crate::hungarian::min_cost_assignment(&cost_matrix, m, n, pad); + for (row, col) in assignment { + let si = avail_snap[row]; + let ti = avail_tree[col]; matched.push((si, ti)); snap_matched[si] = true; tree_available[ti] = false; @@ -291,7 +288,7 @@ fn match_children_for_scoring( } } - // Ambiguous groups: score + greedy assign + // Ambiguous groups: score + optimal assign for (key, snap_indices) in &snap_by_key { let Some(tree_indices) = tree_by_key.get(key) else { continue; @@ -320,32 +317,34 @@ fn match_children_for_scoring( continue; } - let mut pairs: Vec<(u32, usize, usize)> = - Vec::with_capacity(avail_snap.len() * avail_tree.len()); - let mut best_so_far = u32::MAX; + let m = avail_snap.len(); + let n = avail_tree.len(); + let mut cost_matrix = Vec::with_capacity(m); for &si in &avail_snap { + let mut row = Vec::with_capacity(n); for &ti in &avail_tree { let cost = compute_change_count( &snap_children[si], tree_children[ti], tree, - best_so_far, + u32::MAX, depth, session, ); - pairs.push((cost, si, ti)); - if cost < best_so_far { - best_so_far = cost; - } + row.push(cost); } + cost_matrix.push(row); } - pairs.sort_by_key(|&(cost, _, _)| cost); - - for &(_, si, ti) in &pairs { - if snap_matched[si] || tree_matched[ti] { - continue; - } + let assignment = crate::hungarian::min_cost_assignment( + &cost_matrix, + m, + n, + UNMATCHED_PENALTY.saturating_mul(2), + ); + for (row, col) in assignment { + let si = avail_snap[row]; + let ti = avail_tree[col]; matched.push((si, ti)); snap_matched[si] = true; tree_matched[ti] = true; diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index 206fb717..dd10bb32 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -235,7 +235,23 @@ pub fn syncback_dir_no_meta<'sync>( }) .collect(); - // Run the 3-pass matching algorithm to pair new ↔ old children. + // In debug builds, shuffle children to catch order-dependent matching bugs. + #[cfg(debug_assertions)] + let eligible_new = { + use rand::seq::SliceRandom; + let mut v = eligible_new; + v.shuffle(&mut rand::rng()); + v + }; + #[cfg(debug_assertions)] + let eligible_old = { + use rand::seq::SliceRandom; + let mut v = eligible_old; + v.shuffle(&mut rand::rng()); + v + }; + + // Run the matching algorithm to pair new ↔ old children. let matching_session = crate::syncback::matching::MatchingSession::new(); let match_result = match_children( &eligible_new, diff --git a/src/syncback/matching.rs b/src/syncback/matching.rs index 14cde11c..3c2478bd 100644 --- a/src/syncback/matching.rs +++ b/src/syncback/matching.rs @@ -5,8 +5,8 @@ //! //! Algorithm per parent: //! 1. Group by (Name, ClassName) -- 1:1 groups instant-match -//! 2. Ambiguous groups: change-count scoring + greedy assignment -//! (with hash fast-path: hash equal = 0 cost) +//! 2. Ambiguous groups: change-count scoring + optimal assignment +//! via the Hungarian algorithm (with hash fast-path: hash equal = 0 cost) //! //! The change count = how many things the reconciler would need to touch //! to turn instance A into instance B. @@ -71,6 +71,22 @@ pub fn match_children( }; } + // In debug builds, shuffle to catch order-dependent matching bugs. + #[cfg(debug_assertions)] + let new_children = &{ + use rand::seq::SliceRandom; + let mut v = new_children.to_vec(); + v.shuffle(&mut rand::rng()); + v + }; + #[cfg(debug_assertions)] + let old_children = &{ + use rand::seq::SliceRandom; + let mut v = old_children.to_vec(); + v.shuffle(&mut rand::rng()); + v + }; + let new_len = new_children.len(); let old_len = old_children.len(); let mut new_matched = vec![false; new_len]; @@ -127,7 +143,7 @@ pub fn match_children( } // ================================================================ - // Ambiguous groups: change-count scoring + greedy assignment + // Ambiguous groups: change-count scoring + optimal assignment // ================================================================ for (key, new_indices) in &new_by_key { let Some(old_indices) = old_by_key.get(key) else { @@ -158,12 +174,12 @@ pub fn match_children( continue; } - // Score all (A, B) pairs using recursive change-count scoring - let mut pairs: Vec<(u32, usize, usize)> = - Vec::with_capacity(avail_new.len() * avail_old.len()); - let mut best_so_far = u32::MAX; - + // Build cost matrix for the Hungarian algorithm. + let m = avail_new.len(); + let n = avail_old.len(); + let mut cost_matrix = Vec::with_capacity(m); for &ni in &avail_new { + let mut row = Vec::with_capacity(n); for &oi in &avail_old { let cost = compute_change_count( new_children[ni], @@ -172,25 +188,25 @@ pub fn match_children( old_dom, new_hashes, old_hashes, - best_so_far, + u32::MAX, 0, session, ); - pairs.push((cost, ni, oi)); - if cost < best_so_far { - best_so_far = cost; - } + row.push(cost); } + cost_matrix.push(row); } - // Stable sort by cost ascending - pairs.sort_by_key(|&(cost, _, _)| cost); - - // Greedy assign - for &(_, ni, oi) in &pairs { - if new_matched[ni] || old_matched[oi] { - continue; - } + // Optimal assignment via the Hungarian algorithm. + let assignment = crate::hungarian::min_cost_assignment( + &cost_matrix, + m, + n, + UNMATCHED_PENALTY.saturating_mul(2), + ); + for (row, col) in assignment { + let ni = avail_new[row]; + let oi = avail_old[col]; matched.push((ni, oi)); new_matched[ni] = true; old_matched[oi] = true; @@ -310,7 +326,7 @@ fn match_children_for_scoring( } } - // Ambiguous groups: score + greedy assign + // Ambiguous groups: score + optimal assign for (key, new_indices) in &new_by_key { let Some(old_indices) = old_by_key.get(key) else { continue; @@ -339,10 +355,11 @@ fn match_children_for_scoring( continue; } - let mut pairs: Vec<(u32, usize, usize)> = - Vec::with_capacity(avail_new.len() * avail_old.len()); - let mut best_so_far = u32::MAX; + let m = avail_new.len(); + let n = avail_old.len(); + let mut cost_matrix = Vec::with_capacity(m); for &ni in &avail_new { + let mut row = Vec::with_capacity(n); for &oi in &avail_old { let cost = compute_change_count( new_children[ni], @@ -351,23 +368,24 @@ fn match_children_for_scoring( old_dom, new_hashes, old_hashes, - best_so_far, + u32::MAX, depth, session, ); - pairs.push((cost, ni, oi)); - if cost < best_so_far { - best_so_far = cost; - } + row.push(cost); } + cost_matrix.push(row); } - pairs.sort_by_key(|&(cost, _, _)| cost); - - for &(_, ni, oi) in &pairs { - if new_matched[ni] || old_matched[oi] { - continue; - } + let assignment = crate::hungarian::min_cost_assignment( + &cost_matrix, + m, + n, + UNMATCHED_PENALTY.saturating_mul(2), + ); + for (row, col) in assignment { + let ni = avail_new[row]; + let oi = avail_old[col]; matched.push((ni, oi)); new_matched[ni] = true; old_matched[oi] = true; @@ -800,6 +818,8 @@ mod tests { #[test] fn matching_stability() { + use std::collections::HashSet; + let mut new_dom = WeakDom::new(InstanceBuilder::new("DataModel")); let new_root = new_dom.root_ref(); new_dom.insert(new_root, InstanceBuilder::new("Folder").with_name("X")); @@ -833,9 +853,9 @@ mod tests { ); assert_eq!(r1.matched.len(), r2.matched.len()); - for (a, b) in r1.matched.iter().zip(r2.matched.iter()) { - assert_eq!(a, b); - } + let set1: HashSet<(Ref, Ref)> = r1.matched.into_iter().collect(); + let set2: HashSet<(Ref, Ref)> = r2.matched.into_iter().collect(); + assert_eq!(set1, set2); } #[test]