Simplify model builder internal Graph (#1380)

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultTransformerContextBuilder.java

@@ -181,9 +181,7 @@ class DefaultTransformerContextBuilder implements TransformerContextBuilder {
 
     private boolean addEdge(Path from, Path p, DefaultModelProblemCollector problems) {
         try {
-            synchronized (dag) {
-                dag.addEdge(dag.addVertex(from.toString()), dag.addVertex(p.toString()));
-            }
+            dag.addEdge(from.toString(), p.toString());
             return true;
         } catch (Graph.CycleDetectedException e) {
             problems.add(new DefaultModelProblem(

maven-model-builder/src/main/java/org/apache/maven/model/building/Graph.java

@@ -28,70 +28,35 @@ import java.util.List;
 import java.util.Map;
 
 class Graph {
+
+    final Map<String, List<String>> graph = new LinkedHashMap<>();
+
+    synchronized void addEdge(String from, String to) throws CycleDetectedException {
+        graph.computeIfAbsent(from, l -> new ArrayList<>()).add(to);
+        List<String> cycle = visitCycle(graph, Collections.singleton(to), new HashMap<>(), new LinkedList<>());
+        if (cycle != null) {
+            // remove edge which introduced cycle
+            throw new CycleDetectedException(
+                    "Edge between '" + from + "' and '" + to + "' introduces to cycle in the graph", cycle);
+        }
+    }
+
     private enum DfsState {
         VISITING,
         VISITED
     }
 
-    final Map<String, Vertex> vertices = new LinkedHashMap<>();
-
-    public Vertex getVertex(String id) {
-        return vertices.get(id);
-    }
-
-    public Collection<Vertex> getVertices() {
-        return vertices.values();
-    }
-
-    Vertex addVertex(String label) {
-        return vertices.computeIfAbsent(label, Vertex::new);
-    }
-
-    void addEdge(Vertex from, Vertex to) throws CycleDetectedException {
-        from.children.add(to);
-        to.parents.add(from);
-        List<String> cycle = findCycle(to);
-        if (cycle != null) {
-            // remove edge which introduced cycle
-            removeEdge(from, to);
-            throw new CycleDetectedException(
-                    "Edge between '" + from.label + "' and '" + to.label + "' introduces to cycle in the graph", cycle);
-        }
-    }
-
-    void removeEdge(Vertex from, Vertex to) {
-        from.children.remove(to);
-        to.parents.remove(from);
-    }
-
-    List<String> visitAll() {
-        return visitAll(vertices.values(), new HashMap<>(), new ArrayList<>());
-    }
-
-    List<String> findCycle(Vertex vertex) {
-        return visitCycle(Collections.singleton(vertex), new HashMap<>(), new LinkedList<>());
-    }
-
-    private static List<String> visitAll(
-            Collection<Vertex> children, Map<Vertex, DfsState> stateMap, List<String> list) {
-        for (Vertex v : children) {
-            DfsState state = stateMap.putIfAbsent(v, DfsState.VISITING);
-            if (state == null) {
-                visitAll(v.children, stateMap, list);
-                stateMap.put(v, DfsState.VISITED);
-                list.add(v.label);
-            }
-        }
-        return list;
-    }
-
     private static List<String> visitCycle(
-            Collection<Vertex> children, Map<Vertex, DfsState> stateMap, LinkedList<String> cycle) {
-        for (Vertex v : children) {
+            Map<String, List<String>> graph,
+            Collection<String> children,
+            Map<String, DfsState> stateMap,
+            LinkedList<String> cycle) {
+        if (children != null) {
+            for (String v : children) {
                 DfsState state = stateMap.putIfAbsent(v, DfsState.VISITING);
                 if (state == null) {
-                cycle.addLast(v.label);
-                List<String> ret = visitCycle(v.children, stateMap, cycle);
+                    cycle.addLast(v);
+                    List<String> ret = visitCycle(graph, graph.get(v), stateMap, cycle);
                     if (ret != null) {
                         return ret;
                     }
@@ -99,37 +64,16 @@ class Graph {
                     stateMap.put(v, DfsState.VISITED);
                 } else if (state == DfsState.VISITING) {
                     // we are already visiting this vertex, this mean we have a cycle
-                int pos = cycle.lastIndexOf(v.label);
+                    int pos = cycle.lastIndexOf(v);
                     List<String> ret = cycle.subList(pos, cycle.size());
-                ret.add(v.label);
+                    ret.add(v);
                     return ret;
                 }
             }
+        }
         return null;
     }
 
-    static class Vertex {
-        final String label;
-        final List<Vertex> children = new ArrayList<>();
-        final List<Vertex> parents = new ArrayList<>();
-
-        Vertex(String label) {
-            this.label = label;
-        }
-
-        String getLabel() {
-            return label;
-        }
-
-        List<Vertex> getChildren() {
-            return children;
-        }
-
-        List<Vertex> getParents() {
-            return parents;
-        }
-    }
-
     public static class CycleDetectedException extends Exception {
         private final List<String> cycle;
 

maven-model-builder/src/test/java/org/apache/maven/model/building/GraphTest.java

@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.model.building;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class GraphTest {
+
+    @Test
+    void testCycle() throws Graph.CycleDetectedException {
+        Graph graph = new Graph();
+        graph.addEdge("a1", "a2");
+        assertThrows(Graph.CycleDetectedException.class, () -> graph.addEdge("a2", "a1"));
+    }
+
+    @Test
+    public void testPerf() throws IOException {
+        List<String[]> data = new ArrayList<>();
+        String k = null;
+        for (String line : Files.readAllLines(Paths.get("src/test/resources/dag.txt"))) {
+            if (line.startsWith("\t")) {
+                data.add(new String[] {k, line.trim()});
+            } else {
+                k = line;
+            }
+        }
+        Collections.shuffle(data);
+
+        long t0 = System.nanoTime();
+        Graph g = new Graph();
+        data.parallelStream().forEach(s -> {
+            try {
+                g.addEdge(s[0], s[1]);
+            } catch (Exception e) {
+                throw new RuntimeException(e);
+            }
+        });
+        long t1 = System.nanoTime();
+    }
+}