From e297d2b115898ac71ce8c651add2b84610d94194 Mon Sep 17 00:00:00 2001 From: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> Date: Tue, 8 Jul 2025 15:03:28 +0100 Subject: [PATCH 1/7] Remove double method call Signed-off-by: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> --- .../treescanner/NodeRelationshipFinder.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java index 3cfca48d3..72b8fb4b2 100644 --- a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java +++ b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java @@ -126,9 +126,8 @@ private FlowNode getAfterNode(FlowNode node) { @CheckForNull private BlockStartNode getFirstEnclosingNode(FlowNode node) { - return node.getEnclosingBlocks().isEmpty() - ? null - : node.getEnclosingBlocks().get(0); + List enclosingBlocks = node.getEnclosingBlocks(); + return enclosingBlocks.isEmpty() ? null : enclosingBlocks.get(0); } private ArrayDeque getProcessedChildren(@CheckForNull FlowNode node) { From 288b56fd1cb200e76af995d47ee88914424f5380 Mon Sep 17 00:00:00 2001 From: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> Date: Tue, 8 Jul 2025 15:06:05 +0100 Subject: [PATCH 2/7] No need to pass a map, just the values of it are needed Signed-off-by: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> --- .../treescanner/NodeRelationshipFinder.java | 30 +++++++++++-------- .../treescanner/PipelineNodeTreeScanner.java | 2 +- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java index 72b8fb4b2..975205dc7 100644 --- a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java +++ b/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,26 +48,30 @@ public NodeRelationshipFinder() {} * Determines the relationship between FlowNodes {@link FlowNode#getParents()}. */ @NonNull - public LinkedHashMap getNodeRelationships( - @NonNull LinkedHashMap nodeMap) { + public Map getNodeRelationships(@NonNull Collection 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 sortedIds = new ArrayList<>(nodeMap.keySet()); - Collections.sort(sortedIds, new FlowNodeWrapper.NodeIdComparator().reversed()); + List 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; } diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java index 2c8324bc7..f41d0ea18 100644 --- a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java +++ b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java @@ -74,7 +74,7 @@ public void build() { if (execution != null) { LinkedHashMap nodes = getAllNodes(); NodeRelationshipFinder finder = new NodeRelationshipFinder(); - LinkedHashMap relationships = finder.getNodeRelationships(nodes); + Map relationships = finder.getNodeRelationships(nodes.values()); GraphBuilder builder = new GraphBuilder(nodes, relationships, this.run, this.execution); if (isDebugEnabled) { logger.debug("Original nodes:"); From 66098a03746fcf97cefe2f7ae1b47cf710735121 Mon Sep 17 00:00:00 2001 From: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> Date: Tue, 8 Jul 2025 15:08:43 +0100 Subject: [PATCH 3/7] cleanup Signed-off-by: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> --- .../treescanner/NodeRelationshipFinder.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java index 975205dc7..4f54b82b8 100644 --- a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java +++ b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java @@ -77,10 +77,10 @@ public Map getNodeRelationships(@NonNull Collection) { - handleBlockEnd((BlockEndNode) node); + if (node instanceof StepAtomNode atomNode) { + addStepRelationship(atomNode); + } else if (node instanceof BlockEndNode endNode) { + handleBlockEnd(endNode); } else { handleBlockStart(node); } @@ -96,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()); + if (!seenChildNodes.containsKey(node.getEnclosingId())) { + seenChildNodes.put(node.getEnclosingId(), new ArrayDeque<>()); } if (isDebugEnabled) { logger.debug("Adding {} to seenChildNodes {}", node.getId(), node.getEnclosingId()); @@ -135,10 +135,10 @@ private BlockStartNode getFirstEnclosingNode(FlowNode node) { } private ArrayDeque 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(); + return new ArrayDeque<>(); } private void addStepRelationship(@NonNull StepAtomNode step) { From 320d8178320074b97674946f9e4dc1919d78ec1b Mon Sep 17 00:00:00 2001 From: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> Date: Tue, 8 Jul 2025 15:09:59 +0100 Subject: [PATCH 4/7] do we need the extra memory overhead and insertion order guarantees of a linked hash map? Signed-off-by: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> --- .../treescanner/PipelineNodeTreeScanner.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java index f41d0ea18..5b11cc3a0 100644 --- a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java +++ b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java @@ -72,7 +72,7 @@ public void build() { logger.debug("Building graph"); } if (execution != null) { - LinkedHashMap nodes = getAllNodes(); + Map nodes = getAllNodes(); NodeRelationshipFinder finder = new NodeRelationshipFinder(); Map relationships = finder.getNodeRelationships(nodes.values()); GraphBuilder builder = new GraphBuilder(nodes, relationships, this.run, this.execution); @@ -100,13 +100,13 @@ public void build() { /** * Gets all the nodes that are reachable in the graph. */ - private LinkedHashMap getAllNodes() { + private Map getAllNodes() { heads = execution.getCurrentHeads(); final DepthFirstScanner scanner = new DepthFirstScanner(); scanner.setup(heads); // nodes that we've visited - final LinkedHashMap nodeMap = new LinkedHashMap<>(); + final Map nodeMap = new HashMap<>(); for (FlowNode n : scanner) { nodeMap.put(n.getId(), n); From d748e0b9ffbf4d444cf2f99245cf627f5755afd2 Mon Sep 17 00:00:00 2001 From: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> Date: Tue, 8 Jul 2025 15:10:52 +0100 Subject: [PATCH 5/7] cleanup Signed-off-by: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> --- .../treescanner/PipelineNodeTreeScanner.java | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java index 5b11cc3a0..472305115 100644 --- a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java +++ b/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.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -123,7 +123,7 @@ public List 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 +142,7 @@ public Map> getAllSteps() { @NonNull public List getPipelineNodes() { List stageNodes = new ArrayList<>(this.stageNodeMap.values()); - Collections.sort(stageNodes, new FlowNodeWrapper.NodeComparator()); + stageNodes.sort(new FlowNodeWrapper.NodeComparator()); return stageNodes; } @@ -195,9 +195,7 @@ public GraphBuilder( } protected List getNodes() { - return wrappedNodeMap.entrySet().stream() - .map(entrySet -> entrySet.getValue()) - .collect(Collectors.toList()); + return new ArrayList<>(wrappedNodeMap.values()); } /* @@ -307,11 +305,11 @@ private boolean isStartNode(FlowNodeWrapper n) { } Map 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 stageMap = this.getStageMapping(); - List nodeList = new ArrayList(stepMap.values()); - Collections.sort(nodeList, new FlowNodeWrapper.NodeComparator()); + List 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 +339,8 @@ private boolean isExceptionStep(FlowNodeWrapper n) { * Builds a graph from the list of nodes and relationships given to the class. */ private void buildGraph() { - List nodeList = new ArrayList(nodeMap.values()); - Collections.sort(nodeList, new FlowNodeWrapper.FlowNodeComparator()); + List nodeList = new ArrayList<>(nodeMap.values()); + nodeList.sort(new FlowNodeWrapper.FlowNodeComparator()); // 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 && nodeMap.size() <= 2) { if (isDebugEnabled) { logger.debug("getUnhandledException => Returning node: {}", node.getId()); } From 8f60def1d51aa023a1a4be6be8ee417f9a4a91ae Mon Sep 17 00:00:00 2001 From: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> Date: Tue, 8 Jul 2025 15:19:00 +0100 Subject: [PATCH 6/7] the map doesn't seem to be used for lookup, only for the values which are then sorted, as such just use a collection/list Signed-off-by: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> --- .../treescanner/PipelineNodeTreeScanner.java | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java index 472305115..2dc4bf979 100644 --- a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java +++ b/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.HashMap; +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) { - Map nodes = getAllNodes(); + Collection nodes = getAllNodes(); NodeRelationshipFinder finder = new NodeRelationshipFinder(); - Map relationships = finder.getNodeRelationships(nodes.values()); + Map relationships = finder.getNodeRelationships(nodes); GraphBuilder builder = new GraphBuilder(nodes, relationships, this.run, this.execution); if (isDebugEnabled) { logger.debug("Original nodes:"); @@ -100,16 +100,15 @@ public void build() { /** * Gets all the nodes that are reachable in the graph. */ - private Map getAllNodes() { + private List getAllNodes() { heads = execution.getCurrentHeads(); final DepthFirstScanner scanner = new DepthFirstScanner(); scanner.setup(heads); // nodes that we've visited - final Map nodeMap = new HashMap<>(); - + final List nodeMap = new ArrayList<>(); for (FlowNode n : scanner) { - nodeMap.put(n.getId(), n); + nodeMap.add(n); } return nodeMap; } @@ -157,7 +156,7 @@ public boolean isDeclarative() { } private static class GraphBuilder { - private final Map nodeMap; + private final Collection nodes; private final Map relationships; private final WorkflowRun run; @@ -182,11 +181,11 @@ private static class GraphBuilder { * in the same graph. */ public GraphBuilder( - @NonNull Map nodeMap, + Collection nodes, @NonNull Map 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); @@ -339,8 +338,7 @@ private boolean isExceptionStep(FlowNodeWrapper n) { * Builds a graph from the list of nodes and relationships given to the class. */ private void buildGraph() { - List nodeList = new ArrayList<>(nodeMap.values()); - nodeList.sort(new FlowNodeWrapper.FlowNodeComparator()); + List 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; @@ -402,7 +400,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.size() <= 2) { + if (node instanceof BlockEndNode && nodes.size() <= 2) { if (isDebugEnabled) { logger.debug("getUnhandledException => Returning node: {}", node.getId()); } From fdf1a00c841d62f987ed7d2ac437b75b7117430c Mon Sep 17 00:00:00 2001 From: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> Date: Tue, 8 Jul 2025 15:28:39 +0100 Subject: [PATCH 7/7] linting Signed-off-by: Lewis Birks <22620804+lewisbirks@users.noreply.github.com> --- .../treescanner/NodeRelationshipFinder.java | 12 ++++++------ .../treescanner/PipelineNodeTreeScanner.java | 10 ++++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java index 4f54b82b8..461de58c8 100644 --- a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java +++ b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java @@ -51,20 +51,20 @@ public NodeRelationshipFinder() {} public Map getNodeRelationships(@NonNull Collection nodes) { if (isDebugEnabled) { logger.atDebug() - .addArgument(() -> nodes.stream().map(FlowNode::getId).collect(Collectors.joining(", "))) - .log("Original Ids: {}"); + .addArgument(() -> nodes.stream().map(FlowNode::getId).collect(Collectors.joining(", "))) + .log("Original Ids: {}"); } // 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 sorted = nodes.stream() - .sorted(new FlowNodeWrapper.FlowNodeComparator().reversed()) - .toList(); + .sorted(new FlowNodeWrapper.FlowNodeComparator().reversed()) + .toList(); if (isDebugEnabled) { logger.atDebug() - .addArgument(() -> sorted.stream().map(FlowNode::getId).collect(Collectors.joining(", "))) - .log("Sorted Ids: {}"); + .addArgument(() -> sorted.stream().map(FlowNode::getId).collect(Collectors.joining(", "))) + .log("Sorted Ids: {}"); } sorted.forEach(node -> { getRelationshipForNode(node); diff --git a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java index 2dc4bf979..3b20d36bc 100644 --- a/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java +++ b/src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java @@ -106,11 +106,11 @@ private List getAllNodes() { scanner.setup(heads); // nodes that we've visited - final List nodeMap = new ArrayList<>(); + final List nodes = new ArrayList<>(); for (FlowNode n : scanner) { - nodeMap.add(n); + nodes.add(n); } - return nodeMap; + return nodes; } @NonNull @@ -338,7 +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 nodeList = nodes.stream().sorted(new FlowNodeWrapper.FlowNodeComparator()).toList(); + List 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;