Reduce number of calculations when finding a nodes relationships (#882)

Author: lewisbirks

src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java

@@ -5,11 +5,11 @@
 import io.jenkins.plugins.pipelinegraphview.utils.FlowNodeWrapper;
 import io.jenkins.plugins.pipelinegraphview.utils.PipelineNodeUtil;
 import java.util.ArrayDeque;
-import java.util.ArrayList;
-import java.util.Collections;
+import java.util.Collection;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 import org.jenkinsci.plugins.workflow.cps.nodes.StepAtomNode;
 import org.jenkinsci.plugins.workflow.graph.BlockEndNode;
 import org.jenkinsci.plugins.workflow.graph.BlockStartNode;
@@ -48,35 +48,39 @@ public NodeRelationshipFinder() {}
      * Determines the relationship between FlowNodes {@link FlowNode#getParents()}.
      */
     @NonNull
-    public LinkedHashMap<String, NodeRelationship> getNodeRelationships(
-            @NonNull LinkedHashMap<String, FlowNode> nodeMap) {
+    public Map<String, NodeRelationship> getNodeRelationships(@NonNull Collection<FlowNode> nodes) {
         if (isDebugEnabled) {
-            logger.debug("Original Ids: {}", String.join(", ", nodeMap.keySet()));
+            logger.atDebug()
+                    .addArgument(() -> nodes.stream().map(FlowNode::getId).collect(Collectors.joining(", ")))
+                    .log("Original Ids: {}");
         }
-        // This is important, determining the the relationships depends on the order of
+        // This is important, determining the relationships depends on the order of
         // iteration.
         // If there was a method to tell if a node was a parallel block this might be
         // less of an issue.
-        List<String> sortedIds = new ArrayList<>(nodeMap.keySet());
-        Collections.sort(sortedIds, new FlowNodeWrapper.NodeIdComparator().reversed());
+        List<FlowNode> sorted = nodes.stream()
+                .sorted(new FlowNodeWrapper.FlowNodeComparator().reversed())
+                .toList();
         if (isDebugEnabled) {
-            logger.debug("Sorted Ids: {}", String.join(", ", sortedIds));
+            logger.atDebug()
+                    .addArgument(() -> sorted.stream().map(FlowNode::getId).collect(Collectors.joining(", ")))
+                    .log("Sorted Ids: {}");
         }
-        for (String id : sortedIds) {
-            getRelationshipForNode(nodeMap.get(id));
+        sorted.forEach(node -> {
+            getRelationshipForNode(node);
             // Add this node to the parents's stack as the last of it's child nodes that
             //  we have seen.
-            addSeenNodes(nodeMap.get(id));
-        }
+            addSeenNodes(node);
+        });
         return relationships;
     }
 
     private void getRelationshipForNode(@NonNull FlowNode node) {
         // Assign start node to end node.
-        if (node instanceof StepAtomNode) {
-            addStepRelationship((StepAtomNode) node);
-        } else if (node instanceof BlockEndNode<?>) {
-            handleBlockEnd((BlockEndNode<?>) node);
+        if (node instanceof StepAtomNode atomNode) {
+            addStepRelationship(atomNode);
+        } else if (node instanceof BlockEndNode<?> endNode) {
+            handleBlockEnd(endNode);
         } else {
             handleBlockStart(node);
         }
@@ -92,8 +96,8 @@ private void handleBlockStart(@NonNull FlowNode node) {
     }
 
     private void addSeenNodes(FlowNode node) {
-        if (!seenChildNodes.keySet().contains(node.getEnclosingId())) {
-            seenChildNodes.put(node.getEnclosingId(), new ArrayDeque<FlowNode>());
+        if (!seenChildNodes.containsKey(node.getEnclosingId())) {
+            seenChildNodes.put(node.getEnclosingId(), new ArrayDeque<>());
         }
         if (isDebugEnabled) {
             logger.debug("Adding {} to seenChildNodes {}", node.getId(), node.getEnclosingId());
@@ -126,16 +130,15 @@ private FlowNode getAfterNode(FlowNode node) {
 
     @CheckForNull
     private BlockStartNode getFirstEnclosingNode(FlowNode node) {
-        return node.getEnclosingBlocks().isEmpty()
-                ? null
-                : node.getEnclosingBlocks().get(0);
+        List<? extends BlockStartNode> enclosingBlocks = node.getEnclosingBlocks();
+        return enclosingBlocks.isEmpty() ? null : enclosingBlocks.get(0);
     }
 
     private ArrayDeque<FlowNode> getProcessedChildren(@CheckForNull FlowNode node) {
-        if (node != null && seenChildNodes.keySet().contains(node.getId())) {
+        if (node != null && seenChildNodes.containsKey(node.getId())) {
             return seenChildNodes.get(node.getId());
         }
-        return new ArrayDeque<FlowNode>();
+        return new ArrayDeque<>();
     }
 
     private void addStepRelationship(@NonNull StepAtomNode step) {

src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java

@@ -8,7 +8,7 @@
 import io.jenkins.plugins.pipelinegraphview.utils.PipelineNodeUtil;
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Collections;
+import java.util.Collection;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -72,9 +72,9 @@ public void build() {
             logger.debug("Building graph");
         }
         if (execution != null) {
-            LinkedHashMap<String, FlowNode> nodes = getAllNodes();
+            Collection<FlowNode> nodes = getAllNodes();
             NodeRelationshipFinder finder = new NodeRelationshipFinder();
-            LinkedHashMap<String, NodeRelationship> relationships = finder.getNodeRelationships(nodes);
+            Map<String, NodeRelationship> relationships = finder.getNodeRelationships(nodes);
             GraphBuilder builder = new GraphBuilder(nodes, relationships, this.run, this.execution);
             if (isDebugEnabled) {
                 logger.debug("Original nodes:");
@@ -100,18 +100,17 @@ public void build() {
     /**
      * Gets all the nodes that are reachable in the graph.
      */
-    private LinkedHashMap<String, FlowNode> getAllNodes() {
+    private List<FlowNode> getAllNodes() {
         heads = execution.getCurrentHeads();
         final DepthFirstScanner scanner = new DepthFirstScanner();
         scanner.setup(heads);
 
         // nodes that we've visited
-        final LinkedHashMap<String, FlowNode> nodeMap = new LinkedHashMap<>();
-
+        final List<FlowNode> nodes = new ArrayList<>();
         for (FlowNode n : scanner) {
-            nodeMap.put(n.getId(), n);
+            nodes.add(n);
         }
-        return nodeMap;
+        return nodes;
     }
 
     @NonNull
@@ -123,7 +122,7 @@ public List<FlowNodeWrapper> getStageSteps(String startNodeId) {
                 stageSteps.add(wrappedStep);
             }
         }
-        Collections.sort(stageSteps, new FlowNodeWrapper.NodeComparator());
+        stageSteps.sort(new FlowNodeWrapper.NodeComparator());
         if (isDebugEnabled) {
             logger.debug("Returning {} steps for node '{}'", stageSteps.size(), startNodeId);
         }
@@ -142,7 +141,7 @@ public Map<String, List<FlowNodeWrapper>> getAllSteps() {
     @NonNull
     public List<FlowNodeWrapper> getPipelineNodes() {
         List<FlowNodeWrapper> stageNodes = new ArrayList<>(this.stageNodeMap.values());
-        Collections.sort(stageNodes, new FlowNodeWrapper.NodeComparator());
+        stageNodes.sort(new FlowNodeWrapper.NodeComparator());
         return stageNodes;
     }
 
@@ -157,7 +156,7 @@ public boolean isDeclarative() {
     }
 
     private static class GraphBuilder {
-        private final Map<String, FlowNode> nodeMap;
+        private final Collection<FlowNode> nodes;
         private final Map<String, NodeRelationship> relationships;
         private final WorkflowRun run;
 
@@ -182,11 +181,11 @@ private static class GraphBuilder {
          * in the same graph.
          */
         public GraphBuilder(
-                @NonNull Map<String, FlowNode> nodeMap,
+                Collection<FlowNode> nodes,
                 @NonNull Map<String, NodeRelationship> relationships,
                 @NonNull WorkflowRun run,
                 @NonNull FlowExecution execution) {
-            this.nodeMap = nodeMap;
+            this.nodes = nodes;
             this.relationships = relationships;
             this.run = run;
             this.inputAction = run.getAction(InputAction.class);
@@ -195,9 +194,7 @@ public GraphBuilder(
         }
 
         protected List<FlowNodeWrapper> getNodes() {
-            return wrappedNodeMap.entrySet().stream()
-                    .map(entrySet -> entrySet.getValue())
-                    .collect(Collectors.toList());
+            return new ArrayList<>(wrappedNodeMap.values());
         }
 
         /*
@@ -307,11 +304,11 @@ private boolean isStartNode(FlowNodeWrapper n) {
             }
             Map<String, FlowNodeWrapper> stepMap = this.wrappedNodeMap.entrySet().stream()
                     .filter(e -> shouldBeInStepMap(e.getValue()))
-                    .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue()));
+                    .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
 
             Map<String, FlowNodeWrapper> stageMap = this.getStageMapping();
-            List<FlowNodeWrapper> nodeList = new ArrayList<FlowNodeWrapper>(stepMap.values());
-            Collections.sort(nodeList, new FlowNodeWrapper.NodeComparator());
+            List<FlowNodeWrapper> nodeList = new ArrayList<>(stepMap.values());
+            nodeList.sort(new FlowNodeWrapper.NodeComparator());
             for (FlowNodeWrapper step : nodeList) {
                 FlowNodeWrapper firstParent = step.getFirstParent();
                 // Remap parentage of steps that aren't children of stages (e.g. are in Step
@@ -341,8 +338,9 @@ private boolean isExceptionStep(FlowNodeWrapper n) {
          * Builds a graph from the list of nodes and relationships given to the class.
          */
         private void buildGraph() {
-            List<FlowNode> nodeList = new ArrayList<FlowNode>(nodeMap.values());
-            Collections.sort(nodeList, new FlowNodeWrapper.FlowNodeComparator());
+            List<FlowNode> nodeList = nodes.stream()
+                    .sorted(new FlowNodeWrapper.FlowNodeComparator())
+                    .toList();
             // If the Pipeline ended with an unhandled exception, then we want to catch the
             // node which threw it.
             BlockEndNode<?> nodeThatThrewException = null;
@@ -404,7 +402,7 @@ private void buildGraph() {
                  * to the graph, we use the end node that we were given to act as the step
                  * - this might need additional logic when getting the log for the exception.
                  */
-                if (node instanceof BlockEndNode<?> && nodeMap.values().size() <= 2) {
+                if (node instanceof BlockEndNode<?> && nodes.size() <= 2) {
                     if (isDebugEnabled) {
                         logger.debug("getUnhandledException => Returning node: {}", node.getId());
                     }