Skip to content

Commit

Permalink
Reduce needless contention in the include scanner's fileParseCache. The
Browse files Browse the repository at this point in the history
computeIfAbsent() function of ConcurrentHashMap is supposed to have a fast
implementation and the map will then allow ~number of processors concurrent
writes. However, reading a file and extracting the include lines can be a costly
IO operation that does not fit this bill. Use futures instead.

RELNOTES: None.
PiperOrigin-RevId: 355837364
  • Loading branch information
djasper-gh authored and copybara-github committed Feb 5, 2021
1 parent 52c7652 commit d202bd5
Showing 1 changed file with 29 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,11 @@

import com.google.common.base.Preconditions;
import com.google.common.base.Supplier;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.common.flogger.GoogleLogger;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.SettableFuture;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
import com.google.devtools.build.lib.actions.Artifact;
Expand Down Expand Up @@ -56,6 +54,7 @@
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;

/**
* C include scanner. Quickly scans C/C++ source files to determine the bounding set of transitively
Expand All @@ -70,9 +69,6 @@
* </pre>
*/
public class LegacyIncludeScanner implements IncludeScanner {

private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();

private static final class ArtifactWithInclusionContext {
private final Artifact artifact;
private final Kind contextKind;
Expand Down Expand Up @@ -769,48 +765,37 @@ private void process(
throws IOException, ExecException, InterruptedException {
checkForInterrupt("processing", source);

Collection<Inclusion> inclusions;
try {
inclusions =
fileParseCache
.computeIfAbsent(
Collection<Inclusion> inclusions = null;
while (inclusions == null) {
SettableFuture<Collection<Inclusion>> future = SettableFuture.create();
Future<Collection<Inclusion>> previous = fileParseCache.putIfAbsent(source, future);
if (previous == null) {
previous = future;
try {
future.set(
parser.extractInclusions(
source,
file -> {
try {
return Futures.immediateFuture(
parser.extractInclusions(
file,
actionExecutionMetadata,
actionExecutionContext,
grepIncludes,
spawnIncludeScannerSupplier.get(),
isRealOutputFile(source.getExecPath())));
} catch (IOException e) {
throw new IORuntimeException(e);
} catch (ExecException e) {
throw new ExecRuntimeException(e);
} catch (InterruptedException e) {
throw new InterruptedRuntimeException(e);
}
})
.get();
} catch (ExecutionException ee) {
actionExecutionMetadata,
actionExecutionContext,
grepIncludes,
spawnIncludeScannerSupplier.get(),
isRealOutputFile(source.getExecPath())));
} catch (Throwable t) {
fileParseCache.remove(source);
future.setException(t);
throw t;
}
}
try {
Throwables.throwIfInstanceOf(ee.getCause(), RuntimeException.class);
throw new IllegalStateException(ee.getCause());
} catch (IORuntimeException e) {
throw e.getCauseIOException();
} catch (ExecRuntimeException e) {
throw e.getRealCause();
} catch (InterruptedRuntimeException e) {
throw e.getRealCause();
inclusions = Preconditions.checkNotNull(previous.get(), source);
} catch (ExecutionException e) {
// An exception occured when some other thread tried to load the same file that we are
// waiting for. If this is a MissingDepExecException, we have to simply retry as otherwise
// we'd end up in an unexpected state (not requesting any deps, but claiming that there
// are missing ones). For other exceptions, this might not be necessary but is safe to do
// and reduces complexity.
}
} catch (RuntimeException e) {
// TODO(b/175294870): Remove after diagnosing the bug.
logger.atSevere().withCause(e).log("Uncaught exception in call to extractInclusions");
throw e;
}
Preconditions.checkNotNull(inclusions, source);

// Shuffle the inclusions to get better parallelism. See b/62200470.
List<Inclusion> shuffledInclusions = new ArrayList<>(inclusions);
Expand Down

0 comments on commit d202bd5

Please sign in to comment.