Skip to content

Commit

Permalink
[Skymeld] Simplify/Refactor the locks management of IncrementalArtifa…
Browse files Browse the repository at this point in the history
…ctConflictFinder.

- Stopping the synchronization on `freeForAllPool` and instead gracefully handle the `RejectedExecutionException`
- Use a separate lock for all the exclusive portions of the process instead of synchronizing on the executor itself.

PiperOrigin-RevId: 570681920
Change-Id: I7b53a242fc59a740268971ebf5de48eb816ef5f2
  • Loading branch information
joeleba authored and copybara-github committed Oct 4, 2023
1 parent 9a1d385 commit d3e1c67
Showing 1 changed file with 12 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executors;
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.annotation.concurrent.GuardedBy;

Expand All @@ -78,9 +79,13 @@ public final class IncrementalArtifactConflictFinder {
private final AtomicBoolean conflictFound = new AtomicBoolean(false);
private Set<ActionLookupKey> globalVisited = Sets.newConcurrentHashSet();

@GuardedBy("exclusivePool")
@GuardedBy("exclusivePortionLock")
private CountDownLatch nextSignalToWaitFor = null;

// The common lock for the portions of the process where top level targets need to be processed
// exclusively.
private final Object exclusivePortionLock = new Object();

public IncrementalArtifactConflictFinder(
MutableActionGraph threadSafeMutableActionGraph, WalkableGraph walkableGraph) {
this.threadSafeMutableActionGraph = threadSafeMutableActionGraph;
Expand Down Expand Up @@ -241,7 +246,7 @@ ActionConflictsAndStats findArtifactConflicts(
// Only allow 1 top-level target to do ALV collection at a time.
try (SilentCloseable c =
Profiler.instance().profile(ProfilerTask.CONFLICT_CHECK, "ALV collection")) {
synchronized (exclusivePool) {
synchronized (exclusivePortionLock) {
if (!inRerun) {
toWaitFor = nextSignalToWaitFor;
mySignal = new CountDownLatch(1);
Expand Down Expand Up @@ -356,7 +361,7 @@ private void constructActionGraphAndArtifactList(

void shutdown() {
try {
synchronized (exclusivePool) {
synchronized (exclusivePortionLock) {
exclusivePool.awaitQuiescence(true);
}
} catch (InterruptedException e) {
Expand Down Expand Up @@ -565,14 +570,12 @@ public void run() {
pathFragmentTrieRoot,
strictConflictChecks,
badActionMap);
synchronized (freeForAllPool) {
try {
var actionCheckingFuture = freeForAllPool.submit(goThroughActions);
actionCheckingFutures.add(actionCheckingFuture);
} catch (RejectedExecutionException e) {
// Some other thread shut down the executor, exit now. This can happen in the case of an
// analysis error.
if (freeForAllPool.isShutdown()) {
return;
}
// This should be fast, we're only submitting the task here.
actionCheckingFutures.add(freeForAllPool.submit(goThroughActions));
}
}
}
Expand Down

0 comments on commit d3e1c67

Please sign in to comment.