Skip to content
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

[JENKINS-65821] Introducing some synchronisation mechanisms to prevent some race condition #153

Merged
merged 5 commits into from
Jun 7, 2021
Merged

[JENKINS-65821] Introducing some synchronisation mechanisms to prevent some race condition #153

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
61da1b2
Introducing some synchronization mechanisms to try improving performance
twasyl Jun 4, 2021
d13c9a6
Using a lock instead of synchronization
twasyl Jun 4, 2021
689f233
Synchronization at instance level as suggested in PR
twasyl Jun 7, 2021
8f5016c
Revert some unrelated changes
twasyl Jun 7, 2021
4f4aaa5
Revert some unrelated changes
twasyl Jun 7, 2021
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
222 changes: 126 additions & 96 deletions src/main/java/org/jenkinsci/plugins/workflow/graph/StandardGraphLookupView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -24,50 +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<String, String> blockStartToEnd = new HashMap<>();

/** Map a node to its nearest enclosing block */
HashMap<String, String> nearestEnclosingBlock = new HashMap<>();

final ReentrantLock lock = new ReentrantLock();
twasyl marked this conversation as resolved.
Show resolved Hide resolved

public void clearCache() {
blockStartToEnd.clear();
nearestEnclosingBlock.clear();
lock.lock();
try {
blockStartToEnd.clear();
nearestEnclosingBlock.clear();
} finally {
lock.unlock();
twasyl marked this conversation as resolved.
Show resolved Hide resolved
}
}

/** Update with a new node added to the flowgraph */
@Override
public 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<FlowNode> 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
lock.lock();
try {
if (newHead instanceof BlockEndNode) {
blockStartToEnd.put(((BlockEndNode) newHead).getStartNode().getId(), newHead.getId());
String overallEnclosing = nearestEnclosingBlock.get(((BlockEndNode) newHead).getStartNode().getId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sort of thing that makes me dubious you could a lockless data structure for this purpose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dwnusbaum suggests that you probably could, if you wanted to review the logic carefully.

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<FlowNode> 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();
}
}

Expand All @@ -90,24 +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
blockStartToEnd.put(maybeStart.getId(), end.getId());
if (start.equals(maybeStart)) {
return end;
}
} else if (f instanceof BlockStartNode) {
BlockStartNode maybeThis = (BlockStartNode) f;
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;

// 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;
}
Expand All @@ -118,60 +136,68 @@ BlockEndNode bruteForceScanForEnd(@Nonnull BlockStartNode start) {
/** Do a brute-force scan for the enclosing blocks **/
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;
// 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;
}
current = parent;
} finally {
lock.unlock();
}

return null;
}

@CheckForNull
@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());
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;
}
return node;
} finally {
lock.unlock();
}
}

Expand All @@ -181,20 +207,24 @@ public BlockStartNode findEnclosingBlockStart(@Nonnull FlowNode node) {
if (node instanceof FlowStartNode || node instanceof FlowEndNode) {
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);
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);
}
}
}

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;
}
} finally {
lock.unlock();
}
return null;
}
Expand Down