From 61da1b2f1ac9fd3c0e13c7c98b70b5b5c8380316 Mon Sep 17 00:00:00 2001 From: Thierry Wasylczenko Date: Fri, 4 Jun 2021 10:26:35 +0200 Subject: [PATCH 1/5] Introducing some synchronization mechanisms to try improving performance --- .../graph/StandardGraphLookupView.java | 92 +++++++++++-------- 1 file changed, 55 insertions(+), 37 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java b/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java index 5d6e2ffc..215f450f 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java @@ -31,13 +31,17 @@ public final class StandardGraphLookupView implements GraphLookupView, GraphList HashMap nearestEnclosingBlock = new HashMap<>(); public void clearCache() { - blockStartToEnd.clear(); - nearestEnclosingBlock.clear(); + synchronized (blockStartToEnd) { + blockStartToEnd.clear(); + } + synchronized (nearestEnclosingBlock) { + nearestEnclosingBlock.clear(); + } } /** Update with a new node added to the flowgraph */ @Override - public void onNewHead(@Nonnull FlowNode newHead) { + public synchronized void onNewHead(@Nonnull FlowNode newHead) { if (newHead instanceof BlockEndNode) { blockStartToEnd.put(((BlockEndNode)newHead).getStartNode().getId(), newHead.getId()); String overallEnclosing = nearestEnclosingBlock.get(((BlockEndNode) newHead).getStartNode().getId()); @@ -92,10 +96,12 @@ BlockEndNode bruteForceScanForEnd(@Nonnull BlockStartNode start) { scan.setup(start.getExecution().getCurrentHeads()); for (FlowNode f : scan) { if (f instanceof BlockEndNode) { - BlockEndNode end = (BlockEndNode)f; + BlockEndNode end = (BlockEndNode) f; BlockStartNode maybeStart = end.getStartNode(); // Cache start in case we need to scan again in the future - blockStartToEnd.put(maybeStart.getId(), end.getId()); + synchronized (blockStartToEnd) { + blockStartToEnd.put(maybeStart.getId(), end.getId()); + } if (start.equals(maybeStart)) { return end; } @@ -103,7 +109,9 @@ BlockEndNode bruteForceScanForEnd(@Nonnull BlockStartNode start) { BlockStartNode maybeThis = (BlockStartNode) f; // We're walking from the end to the start and see the start without finding the end first, block is incomplete - blockStartToEnd.putIfAbsent(maybeThis.getId(), INCOMPLETE); + synchronized (blockStartToEnd) { + blockStartToEnd.putIfAbsent(maybeThis.getId(), INCOMPLETE); + } if (start.equals(maybeThis)) { // Early exit, the end can't be encountered before the start return null; } @@ -123,19 +131,23 @@ BlockStartNode bruteForceScanForEnclosingBlock(@Nonnull final FlowNode node) { if (current instanceof BlockEndNode) { // Hop over the block to the start BlockStartNode start = ((BlockEndNode) current).getStartNode(); - blockStartToEnd.put(start.getId(), current.getId()); + synchronized (blockStartToEnd) { + blockStartToEnd.put(start.getId(), current.getId()); + } current = start; continue; // Simplifies cases below we only need to look at the immediately preceding node. } // Try for a cache hit if (current != node) { - String enclosingIdFromCache = nearestEnclosingBlock.get(current.getId()); - if (enclosingIdFromCache != null) { - try { - return (BlockStartNode) node.getExecution().getNode(enclosingIdFromCache); - } catch (IOException ioe) { - throw new RuntimeException(ioe); + synchronized (nearestEnclosingBlock) { + String enclosingIdFromCache = nearestEnclosingBlock.get(current.getId()); + if (enclosingIdFromCache != null) { + try { + return (BlockStartNode) node.getExecution().getNode(enclosingIdFromCache); + } catch (IOException ioe) { + throw new RuntimeException(ioe); + } } } } @@ -146,7 +158,9 @@ BlockStartNode bruteForceScanForEnclosingBlock(@Nonnull final FlowNode node) { } FlowNode parent = current.getParents().get(0); if (parent instanceof BlockStartNode) { - nearestEnclosingBlock.put(current.getId(), parent.getId()); + synchronized (nearestEnclosingBlock) { + nearestEnclosingBlock.put(current.getId(), parent.getId()); + } return (BlockStartNode) parent; } current = parent; @@ -159,19 +173,21 @@ BlockStartNode bruteForceScanForEnclosingBlock(@Nonnull final FlowNode node) { @Override public BlockEndNode getEndNode(@Nonnull final BlockStartNode startNode) { - String id = blockStartToEnd.get(startNode.getId()); - if (id != null) { - try { - return id == INCOMPLETE ? null : (BlockEndNode) startNode.getExecution().getNode(id); - } catch (IOException ioe) { - throw new RuntimeException(ioe); - } - } else { - BlockEndNode node = bruteForceScanForEnd(startNode); - if (node != null) { - blockStartToEnd.put(startNode.getId(), node.getId()); + synchronized (blockStartToEnd) { + String id = blockStartToEnd.get(startNode.getId()); + if (id != null) { + try { + return id == INCOMPLETE ? null : (BlockEndNode) startNode.getExecution().getNode(id); + } catch (IOException ioe) { + throw new RuntimeException(ioe); + } + } else { + BlockEndNode node = bruteForceScanForEnd(startNode); + if (node != null) { + blockStartToEnd.put(startNode.getId(), node.getId()); + } + return node; } - return node; } } @@ -182,19 +198,21 @@ public BlockStartNode findEnclosingBlockStart(@Nonnull FlowNode node) { return null; } - String id = nearestEnclosingBlock.get(node.getId()); - if (id != null) { - try { - return (BlockStartNode) node.getExecution().getNode(id); - } catch (IOException ioe) { - throw new RuntimeException(ioe); + synchronized (nearestEnclosingBlock) { + String id = nearestEnclosingBlock.get(node.getId()); + if (id != null) { + try { + return (BlockStartNode) node.getExecution().getNode(id); + } catch (IOException ioe) { + throw new RuntimeException(ioe); + } } - } - BlockStartNode enclosing = bruteForceScanForEnclosingBlock(node); - if (enclosing != null) { - nearestEnclosingBlock.put(node.getId(), enclosing.getId()); - return enclosing; + BlockStartNode enclosing = bruteForceScanForEnclosingBlock(node); + if (enclosing != null) { + nearestEnclosingBlock.put(node.getId(), enclosing.getId()); + return enclosing; + } } return null; } From d13c9a6b1a13c89912fb84cce6fa873e8197783f Mon Sep 17 00:00:00 2001 From: Thierry Wasylczenko Date: Fri, 4 Jun 2021 14:32:11 +0200 Subject: [PATCH 2/5] Using a lock instead of synchronization --- .../graph/StandardGraphLookupView.java | 160 ++++++++++-------- 1 file changed, 86 insertions(+), 74 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java b/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java index 215f450f..e755eb84 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java @@ -13,6 +13,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; +import java.util.concurrent.locks.ReentrantLock; /** * Provides overall insight into the structure of a flow graph... but with limited visibility so we can change implementation. @@ -24,54 +25,62 @@ public final class StandardGraphLookupView implements GraphLookupView, GraphList static final String INCOMPLETE = ""; - /** Map the blockStartNode to its endNode, to accellerate a range of operations */ + /** Map the blockStartNode to its endNode, to accelerate a range of operations */ HashMap blockStartToEnd = new HashMap<>(); /** Map a node to its nearest enclosing block */ HashMap nearestEnclosingBlock = new HashMap<>(); + final ReentrantLock lock = new ReentrantLock(); + public void clearCache() { - synchronized (blockStartToEnd) { + lock.lock(); + try { blockStartToEnd.clear(); - } - synchronized (nearestEnclosingBlock) { nearestEnclosingBlock.clear(); + } finally { + lock.unlock(); } } /** Update with a new node added to the flowgraph */ @Override - public synchronized void onNewHead(@Nonnull FlowNode newHead) { - if (newHead instanceof BlockEndNode) { - blockStartToEnd.put(((BlockEndNode)newHead).getStartNode().getId(), newHead.getId()); - String overallEnclosing = nearestEnclosingBlock.get(((BlockEndNode) newHead).getStartNode().getId()); - if (overallEnclosing != null) { - nearestEnclosingBlock.put(newHead.getId(), overallEnclosing); - } - } else { - if (newHead instanceof BlockStartNode) { - blockStartToEnd.put(newHead.getId(), INCOMPLETE); - } - - // Now we try to generate enclosing block info for caching, by looking at parents - // But we don't try to hard -- usually the cache is populated and we defer recursive walks of the graph - List parents = newHead.getParents(); - if (parents.size() > 0) { - FlowNode parent = parents.get(0); // Multiple parents only for end of a parallel, and then both are BlockEndNodes + public void onNewHead(@Nonnull FlowNode newHead) { + lock.lock(); + try { + if (newHead instanceof BlockEndNode) { + blockStartToEnd.put(((BlockEndNode) newHead).getStartNode().getId(), newHead.getId()); + String overallEnclosing = nearestEnclosingBlock.get(((BlockEndNode) newHead).getStartNode().getId()); + if (overallEnclosing != null) { + nearestEnclosingBlock.put(newHead.getId(), overallEnclosing); + } + } else { + if (newHead instanceof BlockStartNode) { + blockStartToEnd.put(newHead.getId(), INCOMPLETE); + } - if (parent instanceof BlockStartNode) { - nearestEnclosingBlock.put(newHead.getId(), parent.getId()); - } else { - // Try to reuse info from cache for this node: - // If the parent ended a block, we use info from the start of the block since that is at the same block nesting level as our new head - // Otherwise the parent is a normal atom node, and the head is enclosed by the same block - String lookupId = (parent instanceof BlockEndNode) ? ((BlockEndNode) parent).getStartNode().getId() : parent.getId(); - String enclosingId = nearestEnclosingBlock.get(lookupId); - if (enclosingId != null) { - nearestEnclosingBlock.put(newHead.getId(), enclosingId); + // Now we try to generate enclosing block info for caching, by looking at parents + // But we don't try to hard -- usually the cache is populated and we defer recursive walks of the graph + List parents = newHead.getParents(); + if (parents.size() > 0) { + FlowNode parent = parents.get(0); // Multiple parents only for end of a parallel, and then both are BlockEndNodes + + if (parent instanceof BlockStartNode) { + nearestEnclosingBlock.put(newHead.getId(), parent.getId()); + } else { + // Try to reuse info from cache for this node: + // If the parent ended a block, we use info from the start of the block since that is at the same block nesting level as our new head + // Otherwise the parent is a normal atom node, and the head is enclosed by the same block + String lookupId = (parent instanceof BlockEndNode) ? ((BlockEndNode) parent).getStartNode().getId() : parent.getId(); + String enclosingId = nearestEnclosingBlock.get(lookupId); + if (enclosingId != null) { + nearestEnclosingBlock.put(newHead.getId(), enclosingId); + } } } } + } finally { + lock.unlock(); } } @@ -94,28 +103,29 @@ public boolean isActive(@Nonnull FlowNode node) { BlockEndNode bruteForceScanForEnd(@Nonnull BlockStartNode start) { DepthFirstScanner scan = new DepthFirstScanner(); scan.setup(start.getExecution().getCurrentHeads()); - for (FlowNode f : scan) { - if (f instanceof BlockEndNode) { - BlockEndNode end = (BlockEndNode) f; - BlockStartNode maybeStart = end.getStartNode(); - // Cache start in case we need to scan again in the future - synchronized (blockStartToEnd) { + lock.lock(); + try { + for (FlowNode f : scan) { + if (f instanceof BlockEndNode) { + BlockEndNode end = (BlockEndNode) f; + BlockStartNode maybeStart = end.getStartNode(); + // Cache start in case we need to scan again in the future blockStartToEnd.put(maybeStart.getId(), end.getId()); - } - if (start.equals(maybeStart)) { - return end; - } - } else if (f instanceof BlockStartNode) { - BlockStartNode maybeThis = (BlockStartNode) f; + if (start.equals(maybeStart)) { + return end; + } + } else if (f instanceof BlockStartNode) { + BlockStartNode maybeThis = (BlockStartNode) f; - // We're walking from the end to the start and see the start without finding the end first, block is incomplete - synchronized (blockStartToEnd) { + // We're walking from the end to the start and see the start without finding the end first, block is incomplete blockStartToEnd.putIfAbsent(maybeThis.getId(), INCOMPLETE); - } - if (start.equals(maybeThis)) { // Early exit, the end can't be encountered before the start - return null; + if (start.equals(maybeThis)) { // Early exit, the end can't be encountered before the start + return null; + } } } + } finally { + lock.unlock(); } return null; } @@ -126,21 +136,19 @@ BlockEndNode bruteForceScanForEnd(@Nonnull BlockStartNode start) { /** Do a brute-force scan for the enclosing blocks **/ BlockStartNode bruteForceScanForEnclosingBlock(@Nonnull final FlowNode node) { FlowNode current = node; - - while (!(current instanceof FlowStartNode)) { // Hunt back for enclosing blocks, a potentially expensive operation - if (current instanceof BlockEndNode) { - // Hop over the block to the start - BlockStartNode start = ((BlockEndNode) current).getStartNode(); - synchronized (blockStartToEnd) { + lock.lock(); + try { + while (!(current instanceof FlowStartNode)) { // Hunt back for enclosing blocks, a potentially expensive operation + if (current instanceof BlockEndNode) { + // Hop over the block to the start + BlockStartNode start = ((BlockEndNode) current).getStartNode(); blockStartToEnd.put(start.getId(), current.getId()); + current = start; + continue; // Simplifies cases below we only need to look at the immediately preceding node. } - current = start; - continue; // Simplifies cases below we only need to look at the immediately preceding node. - } - // Try for a cache hit - if (current != node) { - synchronized (nearestEnclosingBlock) { + // Try for a cache hit + if (current != node) { String enclosingIdFromCache = nearestEnclosingBlock.get(current.getId()); if (enclosingIdFromCache != null) { try { @@ -150,22 +158,21 @@ BlockStartNode bruteForceScanForEnclosingBlock(@Nonnull final FlowNode node) { } } } - } - // Now see if we have a winner among parents - if (current.getParents().isEmpty()) { - return null; - } - FlowNode parent = current.getParents().get(0); - if (parent instanceof BlockStartNode) { - synchronized (nearestEnclosingBlock) { + // Now see if we have a winner among parents + if (current.getParents().isEmpty()) { + return null; + } + FlowNode parent = current.getParents().get(0); + if (parent instanceof BlockStartNode) { nearestEnclosingBlock.put(current.getId(), parent.getId()); + return (BlockStartNode) parent; } - return (BlockStartNode) parent; + current = parent; } - current = parent; + } finally { + lock.unlock(); } - return null; } @@ -173,7 +180,8 @@ BlockStartNode bruteForceScanForEnclosingBlock(@Nonnull final FlowNode node) { @Override public BlockEndNode getEndNode(@Nonnull final BlockStartNode startNode) { - synchronized (blockStartToEnd) { + lock.lock(); + try { String id = blockStartToEnd.get(startNode.getId()); if (id != null) { try { @@ -188,6 +196,8 @@ public BlockEndNode getEndNode(@Nonnull final BlockStartNode startNode) { } return node; } + } finally { + lock.unlock(); } } @@ -197,8 +207,8 @@ public BlockStartNode findEnclosingBlockStart(@Nonnull FlowNode node) { if (node instanceof FlowStartNode || node instanceof FlowEndNode) { return null; } - - synchronized (nearestEnclosingBlock) { + lock.lock(); + try { String id = nearestEnclosingBlock.get(node.getId()); if (id != null) { try { @@ -213,6 +223,8 @@ public BlockStartNode findEnclosingBlockStart(@Nonnull FlowNode node) { nearestEnclosingBlock.put(node.getId(), enclosing.getId()); return enclosing; } + } finally { + lock.unlock(); } return null; } From 689f233b220f3955747070aaf52331cb1db123b7 Mon Sep 17 00:00:00 2001 From: Thierry Wasylczenko Date: Mon, 7 Jun 2021 15:07:36 +0200 Subject: [PATCH 3/5] Synchronization at instance level as suggested in PR --- .../graph/StandardGraphLookupView.java | 234 ++++++++---------- 1 file changed, 99 insertions(+), 135 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java b/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java index e755eb84..9e3093ff 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java @@ -13,7 +13,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; -import java.util.concurrent.locks.ReentrantLock; /** * Provides overall insight into the structure of a flow graph... but with limited visibility so we can change implementation. @@ -31,56 +30,44 @@ public final class StandardGraphLookupView implements GraphLookupView, GraphList /** Map a node to its nearest enclosing block */ HashMap nearestEnclosingBlock = new HashMap<>(); - final ReentrantLock lock = new ReentrantLock(); - - public void clearCache() { - lock.lock(); - try { - blockStartToEnd.clear(); - nearestEnclosingBlock.clear(); - } finally { - lock.unlock(); - } + public synchronized void clearCache() { + blockStartToEnd.clear(); + nearestEnclosingBlock.clear(); } /** Update with a new node added to the flowgraph */ @Override - public void onNewHead(@Nonnull FlowNode newHead) { - lock.lock(); - try { - if (newHead instanceof BlockEndNode) { - blockStartToEnd.put(((BlockEndNode) newHead).getStartNode().getId(), newHead.getId()); - String overallEnclosing = nearestEnclosingBlock.get(((BlockEndNode) newHead).getStartNode().getId()); - if (overallEnclosing != null) { - nearestEnclosingBlock.put(newHead.getId(), overallEnclosing); - } - } else { - if (newHead instanceof BlockStartNode) { - blockStartToEnd.put(newHead.getId(), INCOMPLETE); - } + public synchronized void onNewHead(@Nonnull FlowNode newHead) { + if (newHead instanceof BlockEndNode) { + blockStartToEnd.put(((BlockEndNode) newHead).getStartNode().getId(), newHead.getId()); + String overallEnclosing = nearestEnclosingBlock.get(((BlockEndNode) newHead).getStartNode().getId()); + if (overallEnclosing != null) { + nearestEnclosingBlock.put(newHead.getId(), overallEnclosing); + } + } else { + if (newHead instanceof BlockStartNode) { + blockStartToEnd.put(newHead.getId(), INCOMPLETE); + } + + // Now we try to generate enclosing block info for caching, by looking at parents + // But we don't try to hard -- usually the cache is populated and we defer recursive walks of the graph + List parents = newHead.getParents(); + if (parents.size() > 0) { + FlowNode parent = parents.get(0); // Multiple parents only for end of a parallel, and then both are BlockEndNodes - // Now we try to generate enclosing block info for caching, by looking at parents - // But we don't try to hard -- usually the cache is populated and we defer recursive walks of the graph - List parents = newHead.getParents(); - if (parents.size() > 0) { - FlowNode parent = parents.get(0); // Multiple parents only for end of a parallel, and then both are BlockEndNodes - - if (parent instanceof BlockStartNode) { - nearestEnclosingBlock.put(newHead.getId(), parent.getId()); - } else { - // Try to reuse info from cache for this node: - // If the parent ended a block, we use info from the start of the block since that is at the same block nesting level as our new head - // Otherwise the parent is a normal atom node, and the head is enclosed by the same block - String lookupId = (parent instanceof BlockEndNode) ? ((BlockEndNode) parent).getStartNode().getId() : parent.getId(); - String enclosingId = nearestEnclosingBlock.get(lookupId); - if (enclosingId != null) { - nearestEnclosingBlock.put(newHead.getId(), enclosingId); - } + if (parent instanceof BlockStartNode) { + nearestEnclosingBlock.put(newHead.getId(), parent.getId()); + } else { + // Try to reuse info from cache for this node: + // If the parent ended a block, we use info from the start of the block since that is at the same block nesting level as our new head + // Otherwise the parent is a normal atom node, and the head is enclosed by the same block + String lookupId = (parent instanceof BlockEndNode) ? ((BlockEndNode) parent).getStartNode().getId() : parent.getId(); + String enclosingId = nearestEnclosingBlock.get(lookupId); + if (enclosingId != null) { + nearestEnclosingBlock.put(newHead.getId(), enclosingId); } } } - } finally { - lock.unlock(); } } @@ -100,131 +87,108 @@ public boolean isActive(@Nonnull FlowNode node) { } // Do a brute-force scan for the block end matching the start, caching info along the way for future use - BlockEndNode bruteForceScanForEnd(@Nonnull BlockStartNode start) { + synchronized BlockEndNode bruteForceScanForEnd(@Nonnull BlockStartNode start) { DepthFirstScanner scan = new DepthFirstScanner(); scan.setup(start.getExecution().getCurrentHeads()); - lock.lock(); - try { - for (FlowNode f : scan) { - if (f instanceof BlockEndNode) { - BlockEndNode end = (BlockEndNode) f; - BlockStartNode maybeStart = end.getStartNode(); - // Cache start in case we need to scan again in the future - blockStartToEnd.put(maybeStart.getId(), end.getId()); - if (start.equals(maybeStart)) { - return end; - } - } else if (f instanceof BlockStartNode) { - BlockStartNode maybeThis = (BlockStartNode) f; + for (FlowNode f : scan) { + if (f instanceof BlockEndNode) { + BlockEndNode end = (BlockEndNode) f; + BlockStartNode maybeStart = end.getStartNode(); + // Cache start in case we need to scan again in the future + blockStartToEnd.put(maybeStart.getId(), end.getId()); + if (start.equals(maybeStart)) { + return end; + } + } else if (f instanceof BlockStartNode) { + BlockStartNode maybeThis = (BlockStartNode) f; - // We're walking from the end to the start and see the start without finding the end first, block is incomplete - blockStartToEnd.putIfAbsent(maybeThis.getId(), INCOMPLETE); - if (start.equals(maybeThis)) { // Early exit, the end can't be encountered before the start - return null; - } + // We're walking from the end to the start and see the start without finding the end first, block is incomplete + blockStartToEnd.putIfAbsent(maybeThis.getId(), INCOMPLETE); + if (start.equals(maybeThis)) { // Early exit, the end can't be encountered before the start + return null; } } - } finally { - lock.unlock(); } return null; } - - - /** Do a brute-force scan for the enclosing blocks **/ - BlockStartNode bruteForceScanForEnclosingBlock(@Nonnull final FlowNode node) { + synchronized BlockStartNode bruteForceScanForEnclosingBlock(@Nonnull final FlowNode node) { FlowNode current = node; - lock.lock(); - try { - while (!(current instanceof FlowStartNode)) { // Hunt back for enclosing blocks, a potentially expensive operation - if (current instanceof BlockEndNode) { - // Hop over the block to the start - BlockStartNode start = ((BlockEndNode) current).getStartNode(); - blockStartToEnd.put(start.getId(), current.getId()); - current = start; - continue; // Simplifies cases below we only need to look at the immediately preceding node. - } + while (!(current instanceof FlowStartNode)) { // Hunt back for enclosing blocks, a potentially expensive operation + if (current instanceof BlockEndNode) { + // Hop over the block to the start + BlockStartNode start = ((BlockEndNode) current).getStartNode(); + blockStartToEnd.put(start.getId(), current.getId()); + current = start; + continue; // Simplifies cases below we only need to look at the immediately preceding node. + } - // Try for a cache hit - if (current != node) { - String enclosingIdFromCache = nearestEnclosingBlock.get(current.getId()); - if (enclosingIdFromCache != null) { - try { - return (BlockStartNode) node.getExecution().getNode(enclosingIdFromCache); - } catch (IOException ioe) { - throw new RuntimeException(ioe); - } + // Try for a cache hit + if (current != node) { + String enclosingIdFromCache = nearestEnclosingBlock.get(current.getId()); + if (enclosingIdFromCache != null) { + try { + return (BlockStartNode) node.getExecution().getNode(enclosingIdFromCache); + } catch (IOException ioe) { + throw new RuntimeException(ioe); } } + } - // Now see if we have a winner among parents - if (current.getParents().isEmpty()) { - return null; - } - FlowNode parent = current.getParents().get(0); - if (parent instanceof BlockStartNode) { - nearestEnclosingBlock.put(current.getId(), parent.getId()); - return (BlockStartNode) parent; - } - current = parent; + // Now see if we have a winner among parents + if (current.getParents().isEmpty()) { + return null; + } + FlowNode parent = current.getParents().get(0); + if (parent instanceof BlockStartNode) { + nearestEnclosingBlock.put(current.getId(), parent.getId()); + return (BlockStartNode) parent; } - } finally { - lock.unlock(); + current = parent; } return null; } @CheckForNull @Override - public BlockEndNode getEndNode(@Nonnull final BlockStartNode startNode) { - - lock.lock(); - try { - String id = blockStartToEnd.get(startNode.getId()); - if (id != null) { - try { - return id == INCOMPLETE ? null : (BlockEndNode) startNode.getExecution().getNode(id); - } catch (IOException ioe) { - throw new RuntimeException(ioe); - } - } else { - BlockEndNode node = bruteForceScanForEnd(startNode); - if (node != null) { - blockStartToEnd.put(startNode.getId(), node.getId()); - } - return node; + public synchronized BlockEndNode getEndNode(@Nonnull final BlockStartNode startNode) { + + String id = blockStartToEnd.get(startNode.getId()); + if (id != null) { + try { + return id == INCOMPLETE ? null : (BlockEndNode) startNode.getExecution().getNode(id); + } catch (IOException ioe) { + throw new RuntimeException(ioe); + } + } else { + BlockEndNode node = bruteForceScanForEnd(startNode); + if (node != null) { + blockStartToEnd.put(startNode.getId(), node.getId()); } - } finally { - lock.unlock(); + return node; } } @CheckForNull @Override - public BlockStartNode findEnclosingBlockStart(@Nonnull FlowNode node) { + public synchronized BlockStartNode findEnclosingBlockStart(@Nonnull FlowNode node) { if (node instanceof FlowStartNode || node instanceof FlowEndNode) { return null; } - lock.lock(); - try { - String id = nearestEnclosingBlock.get(node.getId()); - if (id != null) { - try { - return (BlockStartNode) node.getExecution().getNode(id); - } catch (IOException ioe) { - throw new RuntimeException(ioe); - } + String id = nearestEnclosingBlock.get(node.getId()); + if (id != null) { + try { + return (BlockStartNode) node.getExecution().getNode(id); + } catch (IOException ioe) { + throw new RuntimeException(ioe); } + } - BlockStartNode enclosing = bruteForceScanForEnclosingBlock(node); - if (enclosing != null) { - nearestEnclosingBlock.put(node.getId(), enclosing.getId()); - return enclosing; - } - } finally { - lock.unlock(); + BlockStartNode enclosing = bruteForceScanForEnclosingBlock(node); + if (enclosing != null) { + nearestEnclosingBlock.put(node.getId(), enclosing.getId()); + return enclosing; } return null; } From 8f5016ca2041d3e8053f4e483098de2840ebb097 Mon Sep 17 00:00:00 2001 From: Thierry Wasylczenko Date: Mon, 7 Jun 2021 15:10:25 +0200 Subject: [PATCH 4/5] Revert some unrelated changes --- .../plugins/workflow/graph/StandardGraphLookupView.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java b/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java index 9e3093ff..1c3df128 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java @@ -39,7 +39,7 @@ public synchronized void clearCache() { @Override public synchronized void onNewHead(@Nonnull FlowNode newHead) { if (newHead instanceof BlockEndNode) { - blockStartToEnd.put(((BlockEndNode) newHead).getStartNode().getId(), newHead.getId()); + blockStartToEnd.put(((BlockEndNode)newHead).getStartNode().getId(), newHead.getId()); String overallEnclosing = nearestEnclosingBlock.get(((BlockEndNode) newHead).getStartNode().getId()); if (overallEnclosing != null) { nearestEnclosingBlock.put(newHead.getId(), overallEnclosing); @@ -92,7 +92,7 @@ synchronized BlockEndNode bruteForceScanForEnd(@Nonnull BlockStartNode start) { scan.setup(start.getExecution().getCurrentHeads()); for (FlowNode f : scan) { if (f instanceof BlockEndNode) { - BlockEndNode end = (BlockEndNode) f; + BlockEndNode end = (BlockEndNode)f; BlockStartNode maybeStart = end.getStartNode(); // Cache start in case we need to scan again in the future blockStartToEnd.put(maybeStart.getId(), end.getId()); @@ -115,6 +115,7 @@ synchronized BlockEndNode bruteForceScanForEnd(@Nonnull BlockStartNode start) { /** Do a brute-force scan for the enclosing blocks **/ synchronized BlockStartNode bruteForceScanForEnclosingBlock(@Nonnull final FlowNode node) { FlowNode current = node; + while (!(current instanceof FlowStartNode)) { // Hunt back for enclosing blocks, a potentially expensive operation if (current instanceof BlockEndNode) { // Hop over the block to the start From 4f4aaa5f7feaa3ab986118d844840ee7e43ad27f Mon Sep 17 00:00:00 2001 From: Thierry Wasylczenko Date: Mon, 7 Jun 2021 15:11:51 +0200 Subject: [PATCH 5/5] Revert some unrelated changes --- .../plugins/workflow/graph/StandardGraphLookupView.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java b/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java index 1c3df128..f16ebced 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java @@ -148,6 +148,7 @@ synchronized BlockStartNode bruteForceScanForEnclosingBlock(@Nonnull final FlowN } current = parent; } + return null; } @@ -177,6 +178,7 @@ public synchronized BlockStartNode findEnclosingBlockStart(@Nonnull FlowNode nod if (node instanceof FlowStartNode || node instanceof FlowEndNode) { return null; } + String id = nearestEnclosingBlock.get(node.getId()); if (id != null) { try {