From 917232d556024082003ad3db801028e1495b960a Mon Sep 17 00:00:00 2001 From: Technici4n <13494793+Technici4n@users.noreply.github.com> Date: Fri, 12 Jul 2024 23:06:37 +0200 Subject: [PATCH 1/3] Find more intuitive orderings when there are multiple possible toposorts --- .../fml/loading/toposort/TopologicalSort.java | 89 +++++++++++-------- .../neoforged/fml/TopologicalSortTest.java | 42 +++++++++ 2 files changed, 96 insertions(+), 35 deletions(-) create mode 100644 loader/src/test/java/net/neoforged/fml/TopologicalSortTest.java diff --git a/loader/src/main/java/net/neoforged/fml/loading/toposort/TopologicalSort.java b/loader/src/main/java/net/neoforged/fml/loading/toposort/TopologicalSort.java index 329c88769..ec5aa1888 100644 --- a/loader/src/main/java/net/neoforged/fml/loading/toposort/TopologicalSort.java +++ b/loader/src/main/java/net/neoforged/fml/loading/toposort/TopologicalSort.java @@ -7,15 +7,10 @@ import com.google.common.base.Preconditions; import com.google.common.graph.Graph; -import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Comparator; -import java.util.HashMap; +import java.util.HashSet; import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.PriorityQueue; -import java.util.Queue; import java.util.Set; import org.jetbrains.annotations.Nullable; @@ -26,7 +21,8 @@ * utilized in other fashions, e.g. topology-based registry loading, prioritization * for renderers, and even mod module loading. */ -public final class TopologicalSort { +public final class TopologicalSort { + // TODO: this comment here needs updating, in particular the complexity is now O(VE) I believe /** * A breath-first-search based topological sort. * @@ -61,42 +57,65 @@ public static List topologicalSort(Graph graph, @Nullable Comparator queue = comparator == null ? new ArrayDeque<>() : new PriorityQueue<>(comparator); - final Map degrees = new HashMap<>(); - final List results = new ArrayList<>(); - - for (final T node : graph.nodes()) { - final int degree = graph.inDegree(node); - if (degree == 0) { - queue.add(node); - } else { - degrees.put(node, degree); - } + // Check for cycles + Set> components = new StronglyConnectedComponentDetector<>(graph).getComponents(); + components.removeIf(set -> set.size() < 2); + if (!components.isEmpty()) { + //noinspection unchecked + throw new CyclePresentException((Set>) (Set) components); } - while (!queue.isEmpty()) { - final T current = queue.remove(); - results.add(current); - for (final T successor : graph.successors(current)) { - final int updated = degrees.compute(successor, (node, degree) -> Objects.requireNonNull(degree, () -> "Invalid degree present for " + node) - 1); - if (updated == 0) { - queue.add(successor); - degrees.remove(successor); - } + var toposort = new TopologicalSort<>(graph, comparator); + toposort.resolveSubgraph(new ArrayList<>(graph.nodes())); + return toposort.result; + } + + private final Graph graph; + @Nullable + private final Comparator comparator; + private final Set taken = new HashSet<>(); // nodes that have been added to `result` already + private final List result = new ArrayList<>(); + + private TopologicalSort(Graph graph, @Nullable Comparator comparator) { + this.graph = graph; + this.comparator = comparator; + } + + private void resolveSubgraph(List nodes) { + while (!nodes.isEmpty()) { + var min = removeMin(nodes); + if (!taken.contains(min)) { + // Process all dependencies of `min` + var subGraph = new ArrayList(); + collectChildren(min, subGraph); + resolveSubgraph(subGraph); + // Add `min` + result.add(min); + taken.add(min); } } + } - if (!degrees.isEmpty()) { - Set> components = new StronglyConnectedComponentDetector<>(graph).getComponents(); - components.removeIf(set -> set.size() < 2); - throwCyclePresentException(components); + private T removeMin(List nodes) { + if (this.comparator == null) { + return nodes.removeFirst(); } - return results; + int minIndex = 0; + for (int i = 1; i < nodes.size(); ++i) { + if (this.comparator.compare(nodes.get(i), nodes.get(minIndex)) < 0) { + minIndex = i; + } + } + return nodes.remove(minIndex); } - @SuppressWarnings("unchecked") // for unchecked annotation - private static void throwCyclePresentException(Set> components) { - throw new CyclePresentException((Set>) (Set) components); + private void collectChildren(T node, List out) { + for (var child : this.graph.predecessors(node)) { + if (!this.taken.contains(child)) { + out.add(child); + collectChildren(child, out); + } + } } } diff --git a/loader/src/test/java/net/neoforged/fml/TopologicalSortTest.java b/loader/src/test/java/net/neoforged/fml/TopologicalSortTest.java new file mode 100644 index 000000000..97e6d0400 --- /dev/null +++ b/loader/src/test/java/net/neoforged/fml/TopologicalSortTest.java @@ -0,0 +1,42 @@ +package net.neoforged.fml; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.google.common.graph.ElementOrder; +import com.google.common.graph.GraphBuilder; +import com.google.common.graph.MutableGraph; +import java.util.Comparator; +import net.neoforged.fml.loading.toposort.CyclePresentException; +import net.neoforged.fml.loading.toposort.TopologicalSort; +import org.junit.jupiter.api.Test; + +@SuppressWarnings("UnstableApiUsage") // guava graph is @Beta +public class TopologicalSortTest { + @Test + public void testDefaultOrdering() { + MutableGraph g = GraphBuilder.directed().nodeOrder(ElementOrder.insertion()).build(); + g.putEdge(5, 0); + g.putEdge(5, 4); + g.putEdge(0, 1); + g.putEdge(1, 2); + g.putEdge(0, 4); + g.addNode(3); + + var sorted = TopologicalSort.topologicalSort(g, Comparator.naturalOrder()); + assertThat(sorted) + .containsExactly(5, 0, 1, 2, 3, 4); + } + + @Test + public void testCycle() { + MutableGraph g = GraphBuilder.directed().nodeOrder(ElementOrder.insertion()).build(); + g.putEdge(0, 1); + g.putEdge(1, 3); + g.putEdge(3, 0); + g.addNode(2); + + assertThatThrownBy(() -> TopologicalSort.topologicalSort(g, Comparator.naturalOrder())) + .isInstanceOf(CyclePresentException.class); + } +} From a5d49a44c3f260b19349356e97ea5db30ed8cf26 Mon Sep 17 00:00:00 2001 From: Technici4n <13494793+Technici4n@users.noreply.github.com> Date: Fri, 12 Jul 2024 23:37:39 +0200 Subject: [PATCH 2/3] Fix licenses --- .../src/test/java/net/neoforged/fml/TopologicalSortTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/loader/src/test/java/net/neoforged/fml/TopologicalSortTest.java b/loader/src/test/java/net/neoforged/fml/TopologicalSortTest.java index 97e6d0400..08bf89781 100644 --- a/loader/src/test/java/net/neoforged/fml/TopologicalSortTest.java +++ b/loader/src/test/java/net/neoforged/fml/TopologicalSortTest.java @@ -1,3 +1,8 @@ +/* + * Copyright (c) NeoForged and contributors + * SPDX-License-Identifier: LGPL-2.1-only + */ + package net.neoforged.fml; import static org.assertj.core.api.Assertions.assertThat; From 3454345fcdb5eedbf1e70b3142bb9c96b4427961 Mon Sep 17 00:00:00 2001 From: Technici4n <13494793+Technici4n@users.noreply.github.com> Date: Fri, 12 Jul 2024 23:59:35 +0200 Subject: [PATCH 3/3] Fix accidental cubic worst case --- .../fml/loading/toposort/TopologicalSort.java | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/loader/src/main/java/net/neoforged/fml/loading/toposort/TopologicalSort.java b/loader/src/main/java/net/neoforged/fml/loading/toposort/TopologicalSort.java index ec5aa1888..102b41db8 100644 --- a/loader/src/main/java/net/neoforged/fml/loading/toposort/TopologicalSort.java +++ b/loader/src/main/java/net/neoforged/fml/loading/toposort/TopologicalSort.java @@ -84,15 +84,14 @@ private TopologicalSort(Graph graph, @Nullable Comparator comparat private void resolveSubgraph(List nodes) { while (!nodes.isEmpty()) { var min = removeMin(nodes); - if (!taken.contains(min)) { - // Process all dependencies of `min` - var subGraph = new ArrayList(); - collectChildren(min, subGraph); - resolveSubgraph(subGraph); - // Add `min` - result.add(min); - taken.add(min); - } + // Process all dependencies of `min` + var subGraph = new ArrayList(); + collectChildren(min, subGraph); + resolveSubgraph(subGraph); + nodes.removeIf(taken::contains); + // Add `min` + result.add(min); + taken.add(min); } }