Only process 1 graph request at a time for a given run (#887)

The initial computationally expensive part of creating the graph is creating the PipelineNodeTreeScanner and as such I have created a way of ensuring that all incoming requests use the same parent PipelineNodeGraphAdapter.
This required some additional synchronisation code to ensure that concurrent modifications aren't made to the underlying data.

---------

Signed-off-by: Lewis Birks <[email protected]>

Author: lewisbirks

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

@@ -24,72 +24,81 @@
 public class PipelineNodeGraphAdapter implements PipelineGraphBuilderApi, PipelineStepBuilderApi {
 
     private static final Logger logger = LoggerFactory.getLogger(PipelineNodeGraphAdapter.class);
-    private boolean isDebugEnabled = logger.isDebugEnabled();
-    private PipelineNodeTreeScanner treeScanner;
-    private List<FlowNodeWrapper> pipelineNodesList;
-    private Map<String, List<FlowNodeWrapper>> stepsMap;
-    private Map<String, String> nodesToRemap;
+    private final boolean isDebugEnabled = logger.isDebugEnabled();
+    private final PipelineNodeTreeScanner treeScanner;
+    private volatile List<FlowNodeWrapper> pipelineNodesList;
+    private volatile Map<String, List<FlowNodeWrapper>> stepsMap;
+    private volatile Map<String, String> nodesToRemap;
 
     public PipelineNodeGraphAdapter(WorkflowRun run) {
         treeScanner = new PipelineNodeTreeScanner(run);
     }
 
+    private final Object pipelineLock = new Object();
+    private final Object stepLock = new Object();
+    private final Object remapLock = new Object();
+
     private Map<String, String> getNodesToRemap(List<FlowNodeWrapper> pipelineNodesList) {
         if (this.nodesToRemap != null) {
             return this.nodesToRemap;
         }
-        // Get a map of nodes to remap. The first id is the node to map from, the second
-        // is the node to
-        // map to.
-        // Most of the logic here is to recreate old behavior - it might not be to
-        // everyone's liking.
-        this.nodesToRemap = new HashMap<String, String>();
-        for (int i = pipelineNodesList.size() - 1; i >= 0; i--) {
-            FlowNodeWrapper node = pipelineNodesList.get(i);
-            for (FlowNodeWrapper parent : node.getParents()) {
-                // Parallel Start Nodes that have a Stage with the same name as a parent will be
-                // mapped to that
-                // parent stage
-                // id.
-                if (node.getType() == FlowNodeWrapper.NodeType.PARALLEL_BLOCK
-                        && parent.getType() == FlowNodeWrapper.NodeType.STAGE) {
-                    if (isDebugEnabled) {
-                        logger.debug(
-                                "getNodesToRemap => Found Parallel block {id: {}, name: {}, type: {}} that has a Stage {id: {}, name: {}, type: {}} as a parent. Adding to remap list.",
-                                node.getId(),
-                                node.getDisplayName(),
-                                node.getType(),
-                                parent.getId(),
-                                parent.getDisplayName(),
-                                parent.getType());
+        synchronized (remapLock) {
+            if (this.nodesToRemap != null) {
+                return this.nodesToRemap;
+            }
+            // Get a map of nodes to remap. The first id is the node to map from, the second
+            // is the node to
+            // map to.
+            // Most of the logic here is to recreate old behavior - it might not be to
+            // everyone's liking.
+            Map<String, String> nodesToRemap = new HashMap<>();
+            for (int i = pipelineNodesList.size() - 1; i >= 0; i--) {
+                FlowNodeWrapper node = pipelineNodesList.get(i);
+                for (FlowNodeWrapper parent : node.getParents()) {
+                    // Parallel Start Nodes that have a Stage with the same name as a parent will be
+                    // mapped to that
+                    // parent stage
+                    // id.
+                    if (node.getType() == FlowNodeWrapper.NodeType.PARALLEL_BLOCK
+                            && parent.getType() == FlowNodeWrapper.NodeType.STAGE) {
+                        if (isDebugEnabled) {
+                            logger.debug(
+                                    "getNodesToRemap => Found Parallel block {id: {}, name: {}, type: {}} that has a Stage {id: {}, name: {}, type: {}} as a parent. Adding to remap list.",
+                                    node.getId(),
+                                    node.getDisplayName(),
+                                    node.getType(),
+                                    parent.getId(),
+                                    parent.getDisplayName(),
+                                    parent.getType());
+                        }
+                        nodesToRemap.put(node.getId(), parent.getId());
+                        // Skip other checks.
+                        continue;
                     }
-                    this.nodesToRemap.put(node.getId(), parent.getId());
-                    // Skip other checks.
-                    continue;
-                }
-                // If the node has a parent which is a parallel branch, with the same name and
-                // has only one child (this node)
-                // then remap child nodes to that parent. This removes some superfluous stages
-                // in parallel branches.
-                if (parent.getType() == FlowNodeWrapper.NodeType.PARALLEL
-                        && node.getDisplayName().equals(parent.getDisplayName())) {
-                    if (isDebugEnabled) {
-                        logger.debug(
-                                "getNodesToRemap => Found Stage {id: {}, name: {}, type: {}} that is an only child and has a parent with the same name {id: {}, name: {}, type: {}}. Adding to remap list.",
-                                node.getId(),
-                                node.getDisplayName(),
-                                node.getType(),
-                                parent.getId(),
-                                parent.getDisplayName(),
-                                parent.getType());
+                    // If the node has a parent which is a parallel branch, with the same name and
+                    // has only one child (this node)
+                    // then remap child nodes to that parent. This removes some superfluous stages
+                    // in parallel branches.
+                    if (parent.getType() == FlowNodeWrapper.NodeType.PARALLEL
+                            && node.getDisplayName().equals(parent.getDisplayName())) {
+                        if (isDebugEnabled) {
+                            logger.debug(
+                                    "getNodesToRemap => Found Stage {id: {}, name: {}, type: {}} that is an only child and has a parent with the same name {id: {}, name: {}, type: {}}. Adding to remap list.",
+                                    node.getId(),
+                                    node.getDisplayName(),
+                                    node.getType(),
+                                    parent.getId(),
+                                    parent.getDisplayName(),
+                                    parent.getType());
+                        }
+                        nodesToRemap.put(node.getId(), parent.getId());
                     }
-                    this.nodesToRemap.put(node.getId(), parent.getId());
-                    continue;
                 }
             }
-        }
-        for (String nodeId : this.nodesToRemap.keySet()) {
-            this.nodesToRemap.put(nodeId, getFinalParent(nodeId, this.nodesToRemap));
+            for (String nodeId : nodesToRemap.keySet()) {
+                nodesToRemap.put(nodeId, getFinalParent(nodeId, nodesToRemap));
+            }
+            this.nodesToRemap = nodesToRemap;
         }
         return this.nodesToRemap;
     }
@@ -117,9 +126,7 @@ private void dumpNodeGraphviz(
                     nodes.addAll(stepsList);
                 }
             }
-            if (isDebugEnabled) {
-                logger.debug(FlowNodeWrapper.getNodeGraphviz(nodes));
-            }
+            logger.debug(FlowNodeWrapper.getNodeGraphviz(nodes));
         }
     }
 
@@ -134,26 +141,25 @@ private void remapStageParentage() {
             return;
         }
         Map<String, FlowNodeWrapper> pipelineNodeMap = treeScanner.getPipelineNodeMap();
-
-        this.pipelineNodesList = new ArrayList<FlowNodeWrapper>(pipelineNodeMap.values());
-        Collections.sort(this.pipelineNodesList, new FlowNodeWrapper.NodeComparator());
+        List<FlowNodeWrapper> pipelineNodes = new ArrayList<>(pipelineNodeMap.values());
+        pipelineNodes.sort(new FlowNodeWrapper.NodeComparator());
         // Remove children whose parents were skipped.
-        Map<String, String> nodesToRemap = getNodesToRemap(this.pipelineNodesList);
+        Map<String, String> nodesToRemap = getNodesToRemap(pipelineNodes);
         if (isDebugEnabled) {
             logger.debug(
                     "remapStageParentage => nodesToRemap: {}",
                     nodesToRemap.entrySet().stream()
                             .map(entrySet -> entrySet.getKey() + ":" + entrySet.getValue())
                             .collect(Collectors.joining(",", "[", "]")));
         }
-        dumpNodeGraphviz(this.pipelineNodesList);
+        dumpNodeGraphviz(pipelineNodes);
         // Find all nodes that have a parent to remap (see 'getNodesToRemap') and change
         // their parentage
         // to the designated parent.
-        for (int i = this.pipelineNodesList.size() - 1; i >= 0; i--) {
-            FlowNodeWrapper node = this.pipelineNodesList.get(i);
+        for (int i = pipelineNodes.size() - 1; i >= 0; i--) {
+            FlowNodeWrapper node = pipelineNodes.get(i);
             List<String> parentIds =
-                    node.getParents().stream().map(FlowNodeWrapper::getId).collect(Collectors.toList());
+                    node.getParents().stream().map(FlowNodeWrapper::getId).toList();
             for (String parentId : parentIds) {
                 if (nodesToRemap.containsKey(parentId)) {
                     FlowNodeWrapper parent = pipelineNodeMap.get(parentId);
@@ -193,7 +199,7 @@ private void remapStageParentage() {
             }
         }
         // Remove remapped nodes from the tree
-        this.pipelineNodesList = this.pipelineNodesList.stream()
+        this.pipelineNodesList = pipelineNodes.stream()
                 .
                 // Filter out obsolete Parallel block nodes - ones whose children were remapped
                 // to a
@@ -212,54 +218,57 @@ private void remapStepParentage() {
         if (this.stepsMap != null) {
             return;
         }
-        this.stepsMap = treeScanner.getAllSteps();
-        dumpNodeGraphviz(getPipelineNodes(), this.stepsMap);
-        Map<String, String> nodesToRemap = getNodesToRemap(getPipelineNodes());
+        Map<String, List<FlowNodeWrapper>> stepsMap = treeScanner.getAllSteps();
+        List<FlowNodeWrapper> pipelineNodes = getPipelineNodes();
+        dumpNodeGraphviz(pipelineNodes, stepsMap);
+        Map<String, String> nodesToRemap = getNodesToRemap(pipelineNodes);
         for (Map.Entry<String, String> remapEntry : nodesToRemap.entrySet()) {
             String originalParentId = remapEntry.getKey();
-            if (this.stepsMap.containsKey(originalParentId)) {
+            if (stepsMap.containsKey(originalParentId)) {
                 String remappedParentId = remapEntry.getValue();
                 if (isDebugEnabled) {
                     logger.debug(
                             "remapStepParentage => Remapping {} steps from stage {} to {}.",
-                            this.stepsMap.get(originalParentId).size(),
+                            stepsMap.get(originalParentId).size(),
                             originalParentId,
                             remappedParentId);
                 }
                 List<FlowNodeWrapper> remappedParentStepsList =
-                        this.stepsMap.getOrDefault(remappedParentId, new ArrayList<>());
-                remappedParentStepsList.addAll(this.stepsMap.get(originalParentId));
+                        stepsMap.getOrDefault(remappedParentId, new ArrayList<>());
+                remappedParentStepsList.addAll(stepsMap.get(originalParentId));
                 // Ensure steps are sorted correctly.
-                Collections.sort(remappedParentStepsList, new FlowNodeWrapper.NodeComparator());
-                this.stepsMap.put(remappedParentId, remappedParentStepsList);
-                this.stepsMap.remove(originalParentId);
+                remappedParentStepsList.sort(new FlowNodeWrapper.NodeComparator());
+                stepsMap.put(remappedParentId, remappedParentStepsList);
+                stepsMap.remove(originalParentId);
             }
         }
-        dumpNodeGraphviz(getPipelineNodes(), this.stepsMap);
+        this.stepsMap = stepsMap;
+        dumpNodeGraphviz(pipelineNodes, this.stepsMap);
     }
 
     public List<FlowNodeWrapper> getPipelineNodes() {
         if (this.pipelineNodesList == null) {
-            remapStageParentage();
+            synchronized (pipelineLock) {
+                if (this.pipelineNodesList == null) {
+                    remapStageParentage();
+                }
+            }
         }
-        return this.pipelineNodesList;
+        return Collections.unmodifiableList(this.pipelineNodesList);
     }
 
     public Map<String, List<FlowNodeWrapper>> getAllSteps() {
         if (this.stepsMap == null) {
-            remapStepParentage();
+            synchronized (stepLock) {
+                if (this.stepsMap == null) {
+                    remapStepParentage();
+                }
+            }
         }
-        return this.stepsMap;
+        return Collections.unmodifiableMap(this.stepsMap);
     }
 
     public List<FlowNodeWrapper> getStageSteps(String startNodeId) {
-        return getAllSteps().getOrDefault(startNodeId, new ArrayList<FlowNodeWrapper>());
-    }
-
-    public Map<String, List<FlowNodeWrapper>> getStep() {
-        if (this.stepsMap == null) {
-            remapStepParentage();
-        }
-        return this.stepsMap;
+        return Collections.unmodifiableList(getAllSteps().getOrDefault(startNodeId, new ArrayList<>()));
     }
 }

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

@@ -40,11 +40,6 @@ public class PipelineNodeTreeScanner {
     private final WorkflowRun run;
     private final FlowExecution execution;
 
-    /**
-     * Point in time snapshot of all the active heads.
-     */
-    private List<FlowNode> heads;
-
     // Maps a node ID to a given node wrapper. Stores Stages and parallel blocks -
     // not steps.
     private Map<String, FlowNodeWrapper> stageNodeMap = new LinkedHashMap<>();
@@ -55,7 +50,7 @@ public class PipelineNodeTreeScanner {
     private final boolean declarative;
 
     private static final Logger logger = LoggerFactory.getLogger(PipelineNodeTreeScanner.class);
-    private boolean isDebugEnabled = logger.isDebugEnabled();
+    private final boolean isDebugEnabled = logger.isDebugEnabled();
 
     public PipelineNodeTreeScanner(@NonNull WorkflowRun run) {
         this.run = run;
@@ -101,7 +96,7 @@ public void build() {
      * Gets all the nodes that are reachable in the graph.
      */
     private List<FlowNode> getAllNodes() {
-        heads = execution.getCurrentHeads();
+        List<FlowNode> heads = execution.getCurrentHeads();
         final DepthFirstScanner scanner = new DepthFirstScanner();
         scanner.setup(heads);
 
@@ -115,14 +110,11 @@ private List<FlowNode> getAllNodes() {
 
     @NonNull
     public List<FlowNodeWrapper> getStageSteps(String startNodeId) {
-        List<FlowNodeWrapper> stageSteps = new ArrayList<>();
         FlowNodeWrapper wrappedStage = stageNodeMap.get(startNodeId);
-        for (FlowNodeWrapper wrappedStep : stepNodeMap.values()) {
-            if (wrappedStep.getParents().contains(wrappedStage)) {
-                stageSteps.add(wrappedStep);
-            }
-        }
-        stageSteps.sort(new FlowNodeWrapper.NodeComparator());
+        List<FlowNodeWrapper> stageSteps = stepNodeMap.values().stream()
+                .filter(wrappedStep -> wrappedStep.getParents().contains(wrappedStage))
+                .sorted(new FlowNodeWrapper.NodeComparator())
+                .collect(Collectors.toCollection(ArrayList::new));
         if (isDebugEnabled) {
             logger.debug("Returning {} steps for node '{}'", stageSteps.size(), startNodeId);
         }
@@ -150,7 +142,6 @@ public Map<String, FlowNodeWrapper> getPipelineNodeMap() {
         return this.stageNodeMap;
     }
 
-    @NonNull
     public boolean isDeclarative() {
         return this.declarative;
     }
@@ -163,7 +154,7 @@ private static class GraphBuilder {
         @NonNull
         private final FlowExecution execution;
 
-        private Map<String, FlowNodeWrapper> wrappedNodeMap = new LinkedHashMap<>();
+        private final Map<String, FlowNodeWrapper> wrappedNodeMap = new LinkedHashMap<>();
         // These two are populated when required using by filtering unwanted nodes from
         // 'wrappedNodeMap' into a new map.
         private Map<String, FlowNodeWrapper> wrappedStepMap;
@@ -174,7 +165,7 @@ private static class GraphBuilder {
 
         private final Logger logger = LoggerFactory.getLogger(GraphBuilder.class);
         private final InputAction inputAction;
-        private boolean isDebugEnabled = logger.isDebugEnabled();
+        private final boolean isDebugEnabled = logger.isDebugEnabled();
 
         /*
          * Builds a graph representing this Execution. Stages an steps aer represented
@@ -490,8 +481,8 @@ private void assignParent(@NonNull FlowNodeWrapper wrappedNode, @CheckForNull Fl
         private @NonNull FlowNodeWrapper wrapNode(@NonNull FlowNode node, @NonNull NodeRelationship relationship) {
             TimingInfo timing = null;
             NodeRunStatus status = null;
-            if (relationship instanceof ParallelBlockRelationship && PipelineNodeUtil.isParallelBranch(node)) {
-                ParallelBlockRelationship parallelRelationship = (ParallelBlockRelationship) relationship;
+            if (relationship instanceof ParallelBlockRelationship parallelRelationship
+                    && PipelineNodeUtil.isParallelBranch(node)) {
                 timing = parallelRelationship.getBranchTimingInfo(this.run, (BlockStartNode) node);
                 status = parallelRelationship.getBranchStatus(this.run, (BlockStartNode) node);
             } else {

src/main/java/io/jenkins/plugins/pipelinegraphview/utils/CachedPipelineNodeGraphAdaptor.java

@@ -0,0 +1,38 @@
+package io.jenkins.plugins.pipelinegraphview.utils;
+
+import io.jenkins.plugins.pipelinegraphview.treescanner.PipelineNodeGraphAdapter;
+import java.util.Map;
+import java.util.concurrent.CancellationException;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.ConcurrentHashMap;
+import org.jenkinsci.plugins.workflow.job.WorkflowRun;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class CachedPipelineNodeGraphAdaptor {
+
+    public static final CachedPipelineNodeGraphAdaptor instance = new CachedPipelineNodeGraphAdaptor();
+    private static final Logger log = LoggerFactory.getLogger(CachedPipelineNodeGraphAdaptor.class);
+
+    private final Map<String, CompletableFuture<PipelineNodeGraphAdapter>> tasks = new ConcurrentHashMap<>();
+
+    private CachedPipelineNodeGraphAdaptor() {}
+
+    public PipelineNodeGraphAdapter getFor(WorkflowRun run) {
+        String key = run.getExternalizableId();
+
+        CompletableFuture<PipelineNodeGraphAdapter> task = tasks.computeIfAbsent(key, (ignored) -> {
+            log.debug("Creating new PipelineNodeGraphAdapter for run: {}", key);
+            return CompletableFuture.supplyAsync(() -> new PipelineNodeGraphAdapter(run));
+        });
+
+        try {
+            return task.join();
+        } catch (CancellationException | CompletionException e) {
+            throw new RuntimeException("Failure computing graph for " + key, e);
+        } finally {
+            tasks.remove(key);
+        }
+    }
+}