Skip to content

Reduce number of calculations when finding a nodes relationships #882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
timja merged 7 commits into from
Jul 8, 2025
Merged

Reduce number of calculations when finding a nodes relationships #882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e297d2b
Remove double method call
lewisbirks Jul 8, 2025
288b56f
No need to pass a map, just the values of it are needed
lewisbirks Jul 8, 2025
66098a0
cleanup
lewisbirks Jul 8, 2025
320d817
do we need the extra memory overhead and insertion order guarantees o…
lewisbirks Jul 8, 2025
d748e0b
cleanup
lewisbirks Jul 8, 2025
8f60def
the map doesn't seem to be used for lookup, only for the values which…
lewisbirks Jul 8, 2025
fdf1a00
linting
lewisbirks Jul 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 27 additions & 24 deletions src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/NodeRelationshipFinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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());
Expand Down Expand Up @@ -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) {
Expand Down
42 changes: 20 additions & 22 deletions src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -72,9 +72,9 @@
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:");
Expand All @@ -100,18 +100,17 @@
/**
* 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
Expand All @@ -123,7 +122,7 @@
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);
}
Expand All @@ -142,7 +141,7 @@
@NonNull
public List<FlowNodeWrapper> getPipelineNodes() {
List<FlowNodeWrapper> stageNodes = new ArrayList<>(this.stageNodeMap.values());
Collections.sort(stageNodes, new FlowNodeWrapper.NodeComparator());
stageNodes.sort(new FlowNodeWrapper.NodeComparator());

Check warning on line 144 in src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 144 is not covered by tests
return stageNodes;
}

Expand All @@ -157,7 +156,7 @@
}

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;

Expand All @@ -182,11 +181,11 @@
* 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);
Expand All @@ -195,9 +194,7 @@
}

protected List<FlowNodeWrapper> getNodes() {
return wrappedNodeMap.entrySet().stream()
.map(entrySet -> entrySet.getValue())
.collect(Collectors.toList());
return new ArrayList<>(wrappedNodeMap.values());
}

/*
Expand Down Expand Up @@ -307,11 +304,11 @@
}
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
Expand Down Expand Up @@ -341,8 +338,9 @@
* 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;
Expand Down Expand Up @@ -404,7 +402,7 @@
* 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) {

Check warning on line 405 in src/main/java/io/jenkins/plugins/pipelinegraphview/treescanner/PipelineNodeTreeScanner.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 405 is only partially covered, 2 branches are missing
if (isDebugEnabled) {
logger.debug("getUnhandledException => Returning node: {}", node.getId());
}
Expand Down