From 63928e4ba1ddb8967fd42600709fb97ad1536fb2 Mon Sep 17 00:00:00 2001 From: ulfjack Date: Mon, 6 May 2019 02:44:21 -0700 Subject: [PATCH] Add a futures-based include scanner implementation Enable with --experimental_async_include_scanner This allows integrating with the async spawn execution API in SpawnRunner, reducing the need for a large thread pool. Also, in the legacy implementation, if multiple threads called computeIfAbsent on the same key, then all except one block. This could lead to all threads in the thread pool being blocked, which in turn (due to caller-runs policy) can cause Skyframe evaluator threads to be blocked. However, note that Skyframe threads are still blocked in CppCompileAction.findUsedHeaders even after this change. I have a follow-up change to support an action input discovery API based on continuations. Note that this does not change the include scanner to use continuations. We use continuations elsewhere because we need access to the skyframe function environment, which requires that such code runs in the skyframe thread pool. The include scanner does _not_ need the skyframe function environment, so we can run it in a non-skyframe thread pool. However, we may change this in the future, e.g., to cache include scan results in Skyframe. There was a previous attempt to reimplement include scanning in Skyframe, but that was dropped due to performance overhead. Progress on #6394. PiperOrigin-RevId: 246793955 --- .../lib/includescanning/IncludeParser.java | 69 +++ .../IncludeScannerSupplierImpl.java | 11 +- .../IncludeScanningModule.java | 25 +- .../IncludeScanningOptions.java | 12 + .../includescanning/LegacyIncludeScanner.java | 536 ++++++++++++++++-- .../includescanning/SpawnIncludeScanner.java | 180 +++++- .../includescanning/SwigIncludeScanner.java | 9 +- .../build/lib/rules/cpp/IncludeScanning.java | 4 +- 8 files changed, 788 insertions(+), 58 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeParser.java b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeParser.java index 30e18cadbf6a8d..29324fea93c0e7 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeParser.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeParser.java @@ -24,6 +24,9 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Sets; import com.google.common.io.CharStreams; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.Artifact; @@ -886,6 +889,72 @@ Collection extractInclusions( return ImmutableList.copyOf(inclusions); } + /** + * Extracts all inclusions from a given source file. + * + * @param file the file to parse & extract inclusions from + * @param actionExecutionContext Services in the scope of the action, like the stream to which + * scanning messages are printed + * @return a new set of inclusions, normalized to the cache + */ + ListenableFuture> extractInclusionsAsync( + Artifact file, + ActionExecutionMetadata actionExecutionMetadata, + ActionExecutionContext actionExecutionContext, + Artifact grepIncludes, + @Nullable SpawnIncludeScanner remoteIncludeScanner, + boolean isOutputFile) + throws IOException { + ListenableFuture> inclusions; + if (remoteIncludeScanner != null + && remoteIncludeScanner.shouldParseRemotely(file, actionExecutionContext)) { + inclusions = + remoteIncludeScanner.extractInclusionsAsync( + file, + actionExecutionMetadata, + actionExecutionContext, + grepIncludes, + getFileType(), + isOutputFile); + } else { + try (SilentCloseable c = + Profiler.instance().profile(ProfilerTask.SCANNER, file.getExecPathString())) { + inclusions = + Futures.immediateFuture( + extractInclusions( + FileSystemUtils.readContent(actionExecutionContext.getInputPath(file)))); + } catch (IOException e) { + if (remoteIncludeScanner != null) { + logger.log( + Level.WARNING, + "Falling back on remote parsing of " + actionExecutionContext.getInputPath(file), + e); + inclusions = + remoteIncludeScanner.extractInclusionsAsync( + file, + actionExecutionMetadata, + actionExecutionContext, + grepIncludes, + getFileType(), + isOutputFile); + } else { + throw e; + } + } + } + if (hints != null) { + return Futures.transform( + inclusions, + (c) -> { + // Ugly, but saves doing another copy. + c.addAll(hints.getHintedInclusions(file)); + return c; + }, + MoreExecutors.directExecutor()); + } + return inclusions; + } + /** * Returns type of the scanned file. * diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScannerSupplierImpl.java b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScannerSupplierImpl.java index 4957120ff0b397..f73fcf9159b44f 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScannerSupplierImpl.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScannerSupplierImpl.java @@ -18,6 +18,7 @@ import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; +import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -76,7 +77,7 @@ public boolean equals(Object other) { * Cache of include scan results mapping source paths to sets of scanned inclusions. Shared by all * scanner instances. */ - private final ConcurrentMap> includeParseCache = + private final ConcurrentMap>> includeParseCache = new ConcurrentHashMap<>(); /** Map of grepped include files from input (.cc or .h) to a header-grepped file. */ @@ -84,6 +85,7 @@ public boolean equals(Object other) { private final Supplier spawnIncludeScannerSupplier; private final Path execRoot; + private final boolean useAsyncIncludeScanner; /** Cache of include scanner instances mapped by include-path hashes. */ private final LoadingCache scanners = @@ -102,7 +104,8 @@ public IncludeScanner load(IncludeScannerParams key) { directories.getOutputPath(execRoot.getBaseName()), execRoot, artifactFactory, - spawnIncludeScannerSupplier); + spawnIncludeScannerSupplier, + useAsyncIncludeScanner); } }); @@ -111,13 +114,15 @@ public IncludeScannerSupplierImpl( ExecutorService includePool, ArtifactFactory artifactFactory, Supplier spawnIncludeScannerSupplier, - Path execRoot) { + Path execRoot, + boolean useAsyncIncludeScanner) { this.directories = directories; this.includePool = includePool; this.artifactFactory = artifactFactory; this.spawnIncludeScannerSupplier = spawnIncludeScannerSupplier; this.execRoot = execRoot; this.pathCache = new PathExistenceCache(execRoot, artifactFactory); + this.useAsyncIncludeScanner = useAsyncIncludeScanner; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java index cd4ff724e5928d..76d2ca8f327a91 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.actions.ActionContext; import com.google.devtools.build.lib.actions.ActionExecutionContext; @@ -148,15 +149,19 @@ public static final class SwigIncludeScanningContextImpl implements SwigIncludeS private final CommandEnvironment env; private final Supplier spawnScannerSupplier; private final Supplier includePool; - private final ConcurrentMap> cache = new ConcurrentHashMap<>(); + private final ConcurrentMap>> cache = + new ConcurrentHashMap<>(); + private final boolean useAsyncIncludeScanner; SwigIncludeScanningContextImpl( CommandEnvironment env, Supplier spawnScannerSupplier, - Supplier includePool) { + Supplier includePool, + boolean useAsyncIncludeScanner) { this.env = env; this.spawnScannerSupplier = spawnScannerSupplier; this.includePool = includePool; + this.useAsyncIncludeScanner = useAsyncIncludeScanner; } @Override @@ -177,7 +182,8 @@ public void extractSwigIncludes( swigIncludePaths, env.getDirectories(), env.getSkyframeBuildView().getArtifactFactory(), - env.getExecRoot()); + env.getExecRoot(), + useAsyncIncludeScanner); ImmutableMap.Builder pathToLegalOutputArtifact = ImmutableMap.builder(); for (Artifact path : legalOutputPaths) { @@ -238,7 +244,8 @@ public IncludeScanningActionContextProvider( this.strategies = ImmutableList.of( new CppIncludeExtractionContextImpl(env), - new SwigIncludeScanningContextImpl(env, spawnScannerSupplier, () -> includePool), + new SwigIncludeScanningContextImpl( + env, spawnScannerSupplier, () -> includePool, options.useAsyncIncludeScanner), new CppIncludeScanningContextImpl(() -> includeScannerSupplier)); env.getEventBus().register(this); @@ -270,8 +277,8 @@ public void executionPhaseStarting( @Override public void executorCreated(Iterable usedContexts) throws ExecutorInitException { - int threads = buildRequest.getOptions(IncludeScanningOptions.class) - .includeScanningParallelism; + IncludeScanningOptions options = buildRequest.getOptions(IncludeScanningOptions.class); + int threads = options.includeScanningParallelism; if (threads > 0) { log.info( String.format("Include scanning configured to use a pool with %d threads", threads)); @@ -286,11 +293,11 @@ public void executorCreated(Iterable usedContexts) throws Executo includePool, env.getSkyframeBuildView().getArtifactFactory(), spawnScannerSupplier, - env.getExecRoot()); + env.getExecRoot(), + options.useAsyncIncludeScanner); spawnScannerSupplier.get().setOutputService(env.getOutputService()); - spawnScannerSupplier.get().setInMemoryOutput( - buildRequest.getOptions(IncludeScanningOptions.class).inMemoryIncludesFiles); + spawnScannerSupplier.get().setInMemoryOutput(options.inMemoryIncludesFiles); } } } diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningOptions.java b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningOptions.java index 9fc65b94553ad3..56cdec42691175 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningOptions.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningOptions.java @@ -100,4 +100,16 @@ public ParallelismConverter() throws OptionsParsingException { + " \"auto\" means to use a reasonable value derived from the machine's hardware" + " profile (e.g. the number of processors).") public int includeScanningParallelism; + + @Option( + name = "experimental_async_include_scanner", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = { + OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION, + OptionEffectTag.EXECUTION, + OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS + }, + defaultValue = "false", + help = "Switches to a new implementation of include scanning based on futures.") + public boolean useAsyncIncludeScanner; } diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java index d14bbb209872b2..4d1e2bbd178d6c 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java @@ -15,11 +15,14 @@ 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.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.MoreExecutors; +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; @@ -53,6 +56,7 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; /** @@ -270,7 +274,7 @@ static LocateOnPathResult createNotFound(boolean viewedIllegalOutputFile) { * Externally-scoped cache of file path => parsed inclusion set mappings. Saves us from having to * parse files more than once, and can be shared by scanners with different search paths. */ - private final ConcurrentMap> fileParseCache; + private final ConcurrentMap>> fileParseCache; private final IncludeParser parser; @@ -304,6 +308,8 @@ static LocateOnPathResult createNotFound(boolean viewedIllegalOutputFile) { private final ExecutorService includePool; + private final boolean useAsyncIncludeScanner; + // We are using this Random just for shuffling, so keep the order deterministic by hardcoding // the seed. private static final Random CONSTANT_SEED_RANDOM = new Random(88); @@ -319,14 +325,15 @@ static LocateOnPathResult createNotFound(boolean viewedIllegalOutputFile) { LegacyIncludeScanner( IncludeParser parser, ExecutorService includePool, - ConcurrentMap> cache, + ConcurrentMap>> cache, PathExistenceCache pathCache, List quoteIncludePaths, List includePaths, Path outputPath, Path execRoot, ArtifactFactory artifactFactory, - Supplier spawnIncludeScannerSupplier) { + Supplier spawnIncludeScannerSupplier, + boolean useAsyncIncludeScanner) { this.parser = parser; this.includePool = includePool; this.fileParseCache = cache; @@ -344,6 +351,7 @@ static LocateOnPathResult createNotFound(boolean viewedIllegalOutputFile) { this.includeRootFragment = outputPathFragment.getRelative(BlazeDirectories.RELATIVE_INCLUDE_DIR); this.absoluteRoot = Root.absoluteRoot(execRoot.getFileSystem()); + this.useAsyncIncludeScanner = useAsyncIncludeScanner; } /** @@ -431,7 +439,11 @@ public ListenableFuture processAsync( throws IOException, ExecException, InterruptedException { ImmutableSet pathHints = prepare(actionExecutionContext.getEnvironmentForDiscoveringInputs()); - IncludeVisitor visitor = new IncludeVisitor(includeScanningHeaderData.getModularHeaders()); + IncludeVisitor visitor; + visitor = + useAsyncIncludeScanner + ? new AsyncIncludeVisitor(includeScanningHeaderData.getModularHeaders()) + : new LegacyIncludeVisitor(includeScanningHeaderData.getModularHeaders()); return visitor.processInternal( mainSource, sources, @@ -485,15 +497,29 @@ private boolean isIncPath(PathFragment path) { return path.startsWith(includeRootFragment) && !path.equals(includeRootFragment); } + private interface IncludeVisitor { + ListenableFuture processInternal( + Artifact mainSource, + Collection sources, + IncludeScanningHeaderData includeScanningHeaderData, + List cmdlineIncludes, + Set includes, + ActionExecutionMetadata actionExecutionMetadata, + ActionExecutionContext actionExecutionContext, + Artifact grepIncludes, + ImmutableSet pathHints) + throws InterruptedException, IOException, ExecException; + } + /** * Implements a potentially parallel traversal over source files using a thread pool shared across * different IncludeScanner instances. */ - private class IncludeVisitor extends AbstractQueueVisitor { + private class LegacyIncludeVisitor extends AbstractQueueVisitor implements IncludeVisitor { /** The set of headers known to be part of a C++ module. Scanning can stop here. */ private Set modularHeaders; - public IncludeVisitor(Set modularHeaders) { + public LegacyIncludeVisitor(Set modularHeaders) { super( includePool, /*shutdownOnCompletion=*/ false, @@ -502,7 +528,8 @@ public IncludeVisitor(Set modularHeaders) { this.modularHeaders = modularHeaders; } - ListenableFuture processInternal( + @Override + public ListenableFuture processInternal( Artifact mainSource, Collection sources, IncludeScanningHeaderData includeScanningHeaderData, @@ -643,34 +670,42 @@ private void process( throws IOException, ExecException, InterruptedException { checkForInterrupt("processing", source); - Collection inclusions = null; + Collection inclusions; try { inclusions = - fileParseCache.computeIfAbsent( - source, - file -> { - try { - return 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); - } - }); - } catch (IORuntimeException e) { - throw e.getCauseIOException(); - } catch (ExecRuntimeException e) { - throw e.getRealCause(); - } catch (InterruptedRuntimeException e) { - throw e.getRealCause(); + fileParseCache + .computeIfAbsent( + 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) { + 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(); + } } Preconditions.checkNotNull(inclusions, source); @@ -712,7 +747,7 @@ private void processAsyncIfNotExtracted( Set visitedInclusions, final Artifact grepIncludes) throws IOException, ExecException, InterruptedException { - Collection cacheResult = fileParseCache.get(source); + ListenableFuture> cacheResult = fileParseCache.get(source); if (cacheResult != null) { process( source, @@ -910,6 +945,439 @@ private void processFileLevelHintsAsync( } } + /** + * Implements a potentially parallel traversal over source files using a thread pool shared across + * different IncludeScanner instances. + */ + private class AsyncIncludeVisitor implements IncludeVisitor { + /** The set of headers known to be part of a C++ module. Scanning can stop here. */ + private Set modularHeaders; + + public AsyncIncludeVisitor(Set modularHeaders) { + this.modularHeaders = modularHeaders; + } + + @Override + public ListenableFuture processInternal( + Artifact mainSource, + Collection sources, + IncludeScanningHeaderData includeScanningHeaderData, + List cmdlineIncludes, + Set includes, + ActionExecutionMetadata actionExecutionMetadata, + ActionExecutionContext actionExecutionContext, + Artifact grepIncludes, + ImmutableSet pathHints) + throws InterruptedException, IOException, ExecException { + Set visitedInclusions = Sets.newConcurrentHashSet(); + + try { + ListenableFuture result = Futures.immediateFuture(null); + // Process cmd line includes, if specified. + if (mainSource != null && !cmdlineIncludes.isEmpty()) { + result = + processCmdlineIncludesAsync( + mainSource, + cmdlineIncludes, + grepIncludes, + includes, + includeScanningHeaderData.getPathToLegalOutputArtifact(), + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions); + } + + result = + Futures.transformAsync( + result, + (v) -> + processBulkAsync( + sources, + includes, + includeScanningHeaderData.getPathToLegalOutputArtifact(), + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes), + includePool); + + // Process include hints + // TODO(ulfjack): Make this code go away. Use the new hinted inclusions instead. + Hints hints = parser.getHints(); + if (hints != null) { + // Follow "path" hints. + result = + Futures.transformAsync( + result, + (v) -> + processBulkAsync( + pathHints, + includes, + includeScanningHeaderData.getPathToLegalOutputArtifact(), + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes), + includePool); + result = + Futures.transformAsync( + result, + (v) -> + processAllFileLevelHintsAsync( + hints, + sources, + includes, + includeScanningHeaderData.getPathToLegalOutputArtifact(), + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes), + includePool); + + // Follow "file" hints for all included headers, transitively. + result = + Futures.transformAsync( + result, + (v) -> + processOnePass( + hints, + includes, + includes, + includeScanningHeaderData.getPathToLegalOutputArtifact(), + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes), + includePool); + } + return result; + } catch (IOException | InterruptedException | ExecException | MissingDepException e) { + // Careful: Do not leak visitation threads if we have an exception in the initial thread. + throw e; + } + } + + private ListenableFuture> processOnePass( + Hints hints, + Collection before, + Set includes, + Map pathToLegalOutputArtifact, + ActionExecutionMetadata actionExecutionMetadata, + ActionExecutionContext actionExecutionContext, + Set visitedInclusions, + Artifact grepIncludes) + throws InterruptedException, IOException, ExecException { + Set adjacent = Sets.newConcurrentHashSet(); + ListenableFuture future = + processAllFileLevelHintsAsync( + hints, + before, + adjacent, + pathToLegalOutputArtifact, + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes); + return Futures.transformAsync( + future, + (v) -> { + // Keep novel nodes as the next frontier. + for (Iterator iter = adjacent.iterator(); iter.hasNext(); ) { + if (!includes.add(iter.next())) { + iter.remove(); + } + } + if (adjacent.isEmpty()) { + return Futures.immediateFuture(null); + } + return processOnePass( + hints, + adjacent, + includes, + pathToLegalOutputArtifact, + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes); + }, + includePool); + } + + /** + * Processes a given file for includes and populates the provided set with the visited includes. + * + * @param source the file to process + * @param contextPathPos the position on the include path where the containing file was found, + * or -1 for top-level inclusions + * @param contextKind the kind how the containing file was included, or null for top-level + * inclusions + * @param visited the set to receive the files that are transitively included by {@code source} + * @param pathToLegalOutputArtifact map to look up legal output artifact by path + * @param actionExecutionMetadata owning action + * @param actionExecutionContext Services in the scope of the action, like the stream to which + * @param visitedInclusions the set of all processed inclusions, to avoid processing duplicate + * inclusions. + */ + private ListenableFuture process( + final Artifact source, + int contextPathPos, + Kind contextKind, + Set visited, + Map pathToLegalOutputArtifact, + final ActionExecutionMetadata actionExecutionMetadata, + final ActionExecutionContext actionExecutionContext, + Set visitedInclusions, + final Artifact grepIncludes) + throws IOException, InterruptedException { + checkForInterrupt("processing", source); + + ListenableFuture> actualFuture; + SettableFuture> future = SettableFuture.create(); + ListenableFuture> previous = fileParseCache.putIfAbsent(source, future); + if (previous == null) { + actualFuture = future; + future.setFuture( + parser.extractInclusionsAsync( + source, + actionExecutionMetadata, + actionExecutionContext, + grepIncludes, + spawnIncludeScannerSupplier.get(), + isRealOutputFile(source.getExecPath()))); + // When rewinding, we may need to rerun a spawn that previously failed rather than cache the + // failure here, so we remove the cache entry if the future throws. Unfortunately, we can + // only detect that case by actually calling get(). + future.addListener( + () -> { + try { + future.get(); + } catch (ExecutionException | InterruptedException e) { + fileParseCache.remove(source); + } + }, + MoreExecutors.directExecutor()); + } else { + actualFuture = previous; + } + return Futures.transformAsync( + actualFuture, + (inclusions) -> { + Preconditions.checkNotNull(inclusions, source); + // Shuffle the inclusions to get better parallelism. See b/62200470. + // Be careful not to modify the original collection! It's shared between any number of + // threads. + // TODO: Maybe we should shuffle before returning it to avoid the copy? + List shuffledInclusions = new ArrayList<>(inclusions); + Collections.shuffle(shuffledInclusions, CONSTANT_SEED_RANDOM); + + // For each inclusion: get or locate its target file & recursively process + IncludeScannerHelper helper = + new IncludeScannerHelper(includePaths, quoteIncludePaths, source); + List> allFutures = new ArrayList<>(shuffledInclusions.size()); + for (Inclusion inclusion : shuffledInclusions) { + allFutures.add( + findAndProcess( + helper.createInclusionWithContext(inclusion, contextPathPos, contextKind), + source, + visited, + pathToLegalOutputArtifact, + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes)); + } + return Futures.allAsList(allFutures); + }, + includePool); + } + + /** Visits an inclusion starting from a source file. */ + private ListenableFuture findAndProcess( + InclusionWithContext inclusion, + Artifact source, + Set visited, + Map pathToLegalOutputArtifact, + ActionExecutionMetadata actionExecutionMetadata, + ActionExecutionContext actionExecutionContext, + Set visitedInclusions, + Artifact grepIncludes) + throws IOException, InterruptedException { + // Try to find the included file relative to the file that contains the inclusion. Relative + // inclusions are handled like the first entry on the quote include path + Artifact includeFile = + locateRelative(inclusion.getInclusion(), pathToLegalOutputArtifact, source); + int contextPathPos = 0; + Kind contextKind = null; + + checkForInterrupt("visiting", source); + + // If nothing has been found, get an inclusion from the cache. This will automatically search + // on the include paths and populate the cache if necessary. + if (includeFile == null) { + LocateOnPathResult result = inclusionCache.lookup(inclusion, pathToLegalOutputArtifact); + includeFile = result.path; + contextPathPos = result.includePosition; + contextKind = inclusion.getContextKind(); + } + + // Recursively process the found file (if not yet done). + if (includeFile != null + && !isIllegalOutputFile(includeFile.getExecPath(), pathToLegalOutputArtifact.keySet()) + && visitedInclusions.add( + new ArtifactWithInclusionContext(includeFile, contextKind, contextPathPos))) { + visited.add(includeFile); + if (modularHeaders.contains(includeFile)) { + return Futures.immediateFuture(null); + } + return process( + includeFile, + contextPathPos, + contextKind, + visited, + pathToLegalOutputArtifact, + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes); + } + return Futures.immediateFuture(null); + } + + /** + * Processes a given list of includes for a given base file and populates the provided set with + * the visited includes + * + * @param source the source file used as a reference for finding includes + * @param includes the list of -include option strings to locate and process + * @param visited the set of files that are transitively included by {@code includes} to + * populate + * @param pathToLegalOutputArtifact map to look up legal output artifact by path + * @param actionExecutionContext Services in the scope of the action, like the stream to which + * @param visitedInclusions the set of all processed inclusions, to avoid processing duplicate + * inclusions. + */ + private ListenableFuture processCmdlineIncludesAsync( + Artifact source, + List includes, + Artifact grepIncludes, + Set visited, + Map pathToLegalOutputArtifact, + ActionExecutionMetadata actionExecutionMetadata, + ActionExecutionContext actionExecutionContext, + Set visitedInclusions) + throws IOException, ExecException, InterruptedException { + List> allFutures = new ArrayList<>(includes.size()); + for (String incl : includes) { + InclusionWithContext inclusion = new InclusionWithContext(incl, Kind.QUOTE); + allFutures.add( + findAndProcess( + inclusion, + source, + visited, + pathToLegalOutputArtifact, + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes)); + } + return Futures.allAsList(allFutures); + } + + /** + * Processes a bunch sources asynchronously and adds them and their included files to the + * provided set. + * + * @param sources the files to process and add to the set + * @param visited the set to receive the files that are transitively included by {@code sources} + * @param pathToLegalOutputArtifact map to look up legal output artifact by path + * @param actionExecutionContext Services in the scope of the action, like the stream to which + * @param visitedInclusions the set of all processed inclusions, to avoid processing duplicate + * inclusions. + */ + private ListenableFuture processBulkAsync( + Collection sources, + final Set visited, + final Map pathToLegalOutputArtifact, + final ActionExecutionMetadata actionExecutionMetadata, + final ActionExecutionContext actionExecutionContext, + final Set visitedInclusions, + Artifact grepIncludes) + throws IOException, InterruptedException { + List> allFutures = new ArrayList<>(sources.size()); + for (final Artifact source : sources) { + // TODO(djasper): This looks suspicious. We should only stop based on visitedInclusions. + if (!visited.add(source)) { + continue; + } + + allFutures.add( + process( + source, + /*contextPathPos=*/ -1, + /*contextKind=*/ null, + visited, + pathToLegalOutputArtifact, + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes)); + } + return Futures.allAsList(allFutures); + } + + private ListenableFuture processAllFileLevelHintsAsync( + Hints hints, + Collection sources, + Set alsoVisited, + Map pathToLegalOutputArtifact, + ActionExecutionMetadata actionExecutionMetadata, + ActionExecutionContext actionExecutionContext, + Set visitedInclusions, + Artifact grepIncludes) + throws InterruptedException, IOException, ExecException { + List> allFutures = new ArrayList<>(); + // Follow "file" hints for the primary sources. + for (Artifact source : sources) { + allFutures.add( + processFileLevelHintsAsync( + hints, + source, + alsoVisited, + pathToLegalOutputArtifact, + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes)); + } + return Futures.allAsList(allFutures); + } + + private ListenableFuture processFileLevelHintsAsync( + final Hints hints, + final Artifact include, + final Set alsoVisited, + final Map pathToLegalOutputArtifact, + final ActionExecutionMetadata actionExecutionMetadata, + final ActionExecutionContext actionExecutionContext, + final Set visitedInclusions, + Artifact grepIncludes) + throws InterruptedException, IOException, ExecException { + Collection sources = hints.getFileLevelHintedInclusionsLegacy(include); + // Early-out if there's nothing to do to avoid enqueuing a closure + if (sources.isEmpty()) { + return Futures.immediateFuture(null); + } + return processBulkAsync( + sources, + alsoVisited, + pathToLegalOutputArtifact, + actionExecutionMetadata, + actionExecutionContext, + visitedInclusions, + grepIncludes); + } + } + private static class ExecRuntimeException extends RuntimeException { private final ExecException cause; diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java index 8111a6628cf975..023285aa9ad90c 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java @@ -19,6 +19,10 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.MoreExecutors; +import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.actions.AbstractAction; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionExecutionContext; @@ -36,11 +40,13 @@ import com.google.devtools.build.lib.actions.SimpleSpawn; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnActionContext; +import com.google.devtools.build.lib.actions.SpawnContinuation; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.includescanning.IncludeParser.GrepIncludesFileType; import com.google.devtools.build.lib.includescanning.IncludeParser.Inclusion; import com.google.devtools.build.lib.util.io.FileOutErr; +import com.google.devtools.build.lib.vfs.IORuntimeException; import com.google.devtools.build.lib.vfs.OutputService; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -358,12 +364,13 @@ static InputStream spawnGrep( FileOutErr originalOutErr = actionExecutionContext.getFileOutErr(); FileOutErr grepOutErr = originalOutErr.childOutErr(); SpawnActionContext context = actionExecutionContext.getContext(SpawnActionContext.class); + ActionExecutionContext spawnContext = actionExecutionContext.withFileOutErr(grepOutErr); List results; try { - results = context.exec(spawn, actionExecutionContext.withFileOutErr(grepOutErr)); - dump(actionExecutionContext, grepOutErr, originalOutErr); + results = context.exec(spawn, spawnContext); + dump(spawnContext, actionExecutionContext); } catch (ExecException e) { - dump(actionExecutionContext, grepOutErr, originalOutErr); + dump(spawnContext, actionExecutionContext); throw e; } @@ -371,10 +378,169 @@ static InputStream spawnGrep( return result.getInMemoryOutput(output); } - private static void dump(ActionExecutionContext parentContext, FileOutErr from, FileOutErr to) { - if (from.hasRecordedOutput()) { - synchronized (parentContext) { - FileOutErr.dump(from, to); + /** Extracts and returns inclusions from "file" using a spawn. */ + public ListenableFuture> extractInclusionsAsync( + Artifact file, + ActionExecutionMetadata actionExecutionMetadata, + ActionExecutionContext actionExecutionContext, + Artifact grepIncludes, + GrepIncludesFileType fileType, + boolean placeNextToFile) + throws IOException { + Path output = + getIncludesOutput( + file, actionExecutionContext.getPathResolver(), fileType, placeNextToFile); + if (!inMemoryOutput) { + AbstractAction.deleteOutput(output, placeNextToFile ? file.getRoot() : null); + if (!placeNextToFile) { + output.getParentDirectory().createDirectoryAndParents(); + } + } + + ListenableFuture dotIncludeStreamFuture = + spawnGrepAsync( + file, + execPath(output), + inMemoryOutput, + // We use {@link GrepIncludesAction} primarily to overwrite {@link Action#getMnemonic}. + // You might be tempted to use a custom mnemonic on the Spawn instead, but rest assured + // that _this does not work_. We call Spawn.getResourceOwner().getMnemonic() in a lot of + // places, some of which are downstream from here, and doing so would cause the Spawn + // and its owning ActionExecutionMetadata to be inconsistent with each other. + new GrepIncludesAction(actionExecutionMetadata, file.getExecPath()), + actionExecutionContext, + grepIncludes, + fileType); + return Futures.transform( + dotIncludeStreamFuture, + (stream) -> { + try { + return IncludeParser.processIncludes(output, stream); + } catch (IOException e) { + throw new IORuntimeException(e); + } + }, + MoreExecutors.directExecutor()); + } + + /** + * Executes grep-includes. + * + * @param input the file to parse + * @param outputExecPath the output file (exec path) + * @param inMemoryOutput if true, return the contents of the output in the return value instead of + * to the given Path + * @param resourceOwner the resource owner + * @param actionExecutionContext services in the scope of the action. Like the Err/Out stream + * outputs. + * @param fileType Either "c++" or "swig", passed verbatim to grep-includes. + * @return The InputStream of the .includes file if inMemoryOutput feature retrieved it directly. + * Otherwise "null" + * @throws ExecException if scanning fails + */ + private static ListenableFuture spawnGrepAsync( + Artifact input, + PathFragment outputExecPath, + boolean inMemoryOutput, + ActionExecutionMetadata resourceOwner, + ActionExecutionContext actionExecutionContext, + Artifact grepIncludes, + GrepIncludesFileType fileType) { + ActionInput output = ActionInputHelper.fromPath(outputExecPath); + ImmutableList inputs = ImmutableList.of(grepIncludes, input); + ImmutableList outputs = ImmutableList.of(output); + ImmutableList command = + ImmutableList.of( + grepIncludes.getExecPathString(), + input.getExecPath().getPathString(), + outputExecPath.getPathString(), + fileType.getFileType()); + + ImmutableMap.Builder execInfoBuilder = ImmutableMap.builder(); + if (inMemoryOutput) { + execInfoBuilder.put( + ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS, outputExecPath.getPathString()); + } + execInfoBuilder.put(ExecutionRequirements.DO_NOT_REPORT, ""); + + Spawn spawn = + new SimpleSpawn( + resourceOwner, + command, + ImmutableMap.of(), + execInfoBuilder.build(), + inputs, + outputs, + LOCAL_RESOURCES); + + actionExecutionContext.maybeReportSubcommand(spawn); + + // Sharing the originalOutErr across spawnGrep calls would not be thread-safe. Instead, write + // outerr to a temporary location and copy it back to the original after execution, using the + // parent context as a lock to make it thread-safe (see dump() below). + FileOutErr originalOutErr = actionExecutionContext.getFileOutErr(); + FileOutErr grepOutErr = originalOutErr.childOutErr(); + SettableFuture future = SettableFuture.create(); + ActionExecutionContext grepContext = actionExecutionContext.withFileOutErr(grepOutErr); + try { + process( + future, + SpawnContinuation.ofBeginExecution(spawn, grepContext).execute(), + output, + grepContext, + actionExecutionContext); + } catch (ExecException e) { + dump(grepContext, actionExecutionContext); + future.setException(e); + } catch (InterruptedException e) { + dump(grepContext, actionExecutionContext); + future.cancel(false); + } + return future; + } + + private static void process( + SettableFuture future, + SpawnContinuation continuation, + ActionInput output, + ActionExecutionContext actionExecutionContext, + ActionExecutionContext originalActionExecutionContext) { + if (continuation.isDone()) { + List results = continuation.get(); + dump(actionExecutionContext, originalActionExecutionContext); + SpawnResult result = Iterables.getLast(results); + InputStream stream = result.getInMemoryOutput(output); + try { + future.set( + stream == null ? actionExecutionContext.getInputPath(output).getInputStream() : stream); + } catch (IOException e) { + future.setException(e); + } + } else { + continuation + .getFuture() + .addListener( + () -> { + try { + SpawnContinuation next = continuation.execute(); + process( + future, next, output, actionExecutionContext, originalActionExecutionContext); + } catch (ExecException e) { + dump(actionExecutionContext, originalActionExecutionContext); + future.setException(e); + } catch (InterruptedException e) { + dump(actionExecutionContext, originalActionExecutionContext); + future.cancel(false); + } + }, + MoreExecutors.directExecutor()); + } + } + + private static void dump(ActionExecutionContext fromContext, ActionExecutionContext toContext) { + if (fromContext.getFileOutErr().hasRecordedOutput()) { + synchronized (toContext) { + FileOutErr.dump(fromContext.getFileOutErr(), toContext.getFileOutErr()); } } } diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/SwigIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/SwigIncludeScanner.java index f33dd026f21537..990a740048d641 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/SwigIncludeScanner.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/SwigIncludeScanner.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.includescanning; +import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -40,11 +41,12 @@ public class SwigIncludeScanner extends LegacyIncludeScanner { public SwigIncludeScanner( ExecutorService includePool, SpawnIncludeScanner spawnIncludeScanner, - ConcurrentMap> cache, + ConcurrentMap>> cache, List includePaths, BlazeDirectories directories, ArtifactFactory artifactFactory, - Path execRoot) { + Path execRoot, + boolean useAsyncIncludeScanner) { super( new SwigIncludeParser(), includePool, @@ -55,6 +57,7 @@ public SwigIncludeScanner( directories.getOutputPath(execRoot.getBaseName()), execRoot, artifactFactory, - () -> spawnIncludeScanner); + () -> spawnIncludeScanner, + useAsyncIncludeScanner); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanning.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanning.java index 4fc1ab47cd0359..80bbda70716a54 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanning.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanning.java @@ -91,9 +91,9 @@ public ListenableFuture> determineAdditionalInputs( Artifact mainSource = action.getMainIncludeScannerSource(); Collection sources = action.getIncludeScannerSources(); - Profiler profiler = Profiler.instance(); try (SilentCloseable c = - profiler.profile(ProfilerTask.SCANNER, action.getSourceFile().getExecPathString())) { + Profiler.instance() + .profile(ProfilerTask.SCANNER, action.getSourceFile().getExecPathString())) { ListenableFuture future = scanner.processAsync( mainSource,