Skip to content

Commit

Permalink
Reduce garbage in header discovery.
Browse files Browse the repository at this point in the history
The `ImmutableMap` copy creates significant garbage in some builds. Since `HeaderDiscovery` instances are only constructed for one-time use in practice, make `discoverInputsFromDependencies` a static method.

The set of tree artifacts is dropped in favor of just a map because the `NestedSet` from which they originate will already deduplicate. Additionally, it is not necessary to also store them in the main derived artifact map.

PiperOrigin-RevId: 381027740
  • Loading branch information
justinhorvitz authored and copybara-github committed Jun 23, 2021
1 parent 3b6274c commit a617b5a
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 222 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.rules.cpp;

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.nio.charset.StandardCharsets.ISO_8859_1;

import com.google.common.annotations.VisibleForTesting;
Expand Down Expand Up @@ -160,7 +161,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
* The fingerprint of {@link #compileCommandLine}. This is computed lazily so that the command
* line is not unnecessarily flattened outside of action execution.
*/
byte[] commandLineKey;
private byte[] commandLineKey;

private final ImmutableMap<String, String> executionInfo;
private final String actionName;
Expand Down Expand Up @@ -787,7 +788,7 @@ private List<PathFragment> getSystemIncludeDirs(List<String> compilerOptions) {
return result.build();
}

private List<String> getCmdlineIncludes(List<String> args) {
private static ImmutableList<String> getCmdlineIncludes(List<String> args) {
ImmutableList.Builder<String> cmdlineIncludes = ImmutableList.builder();
for (Iterator<String> argi = args.iterator(); argi.hasNext(); ) {
String arg = argi.next();
Expand Down Expand Up @@ -879,9 +880,7 @@ public List<String> getArguments() throws CommandLineExpansionException {
@Override
public Sequence<CommandLineArgsApi> getStarlarkArgs() throws EvalException {
ImmutableSet<Artifact> directoryInputs =
getInputs().toList().stream()
.filter(artifact -> artifact.isDirectory())
.collect(ImmutableSet.toImmutableSet());
getInputs().toList().stream().filter(Artifact::isDirectory).collect(toImmutableSet());

CommandLine commandLine = compileCommandLine.getFilteredFeatureConfigurationCommandLine(this);
ParamFileInfo paramFileInfo = null;
Expand Down Expand Up @@ -1052,7 +1051,7 @@ public void validateInclusions(
errors.assertProblemFree(this, getSourceFile());
}

Iterable<PathFragment> getValidationIgnoredDirs() {
private Iterable<PathFragment> getValidationIgnoredDirs() {
List<PathFragment> cxxSystemIncludeDirs = getBuiltInIncludeDirectories();
return Iterables.concat(cxxSystemIncludeDirs, ccCompilationContext.getSystemIncludeDirs());
}
Expand Down Expand Up @@ -1547,32 +1546,28 @@ protected Spawn createSpawn(Path execRoot, Map<String, String> clientEnv)
}
}

@VisibleForTesting
public NestedSet<Artifact> discoverInputsFromShowIncludesFilters(
private NestedSet<Artifact> discoverInputsFromShowIncludesFilters(
Path execRoot,
ArtifactResolver artifactResolver,
ShowIncludesFilter showIncludesFilterForStdout,
ShowIncludesFilter showIncludesFilterForStderr,
boolean siblingRepositoryLayout)
throws ActionExecutionException {
ImmutableList.Builder<Path> dependencies = new ImmutableList.Builder<>();
dependencies.addAll(showIncludesFilterForStdout.getDependencies(execRoot));
dependencies.addAll(showIncludesFilterForStderr.getDependencies(execRoot));
HeaderDiscovery.Builder discoveryBuilder =
new HeaderDiscovery.Builder()
.setAction(this)
.setSourceFile(getSourceFile())
.setDependencies(dependencies.build())
.setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot))
.setAllowedDerivedInputs(getAllowedDerivedInputs());

if (needsIncludeValidation) {
discoveryBuilder.shouldValidateInclusions();
}

return discoveryBuilder
.build()
.discoverInputsFromDependencies(execRoot, artifactResolver, siblingRepositoryLayout);
Collection<Path> stdoutDeps = showIncludesFilterForStdout.getDependencies(execRoot);
Collection<Path> stderrDeps = showIncludesFilterForStderr.getDependencies(execRoot);
return HeaderDiscovery.discoverInputsFromDependencies(
this,
getSourceFile(),
needsIncludeValidation,
ImmutableList.<Path>builderWithExpectedSize(stdoutDeps.size() + stderrDeps.size())
.addAll(stdoutDeps)
.addAll(stderrDeps)
.build(),
getPermittedSystemIncludePrefixes(execRoot),
getAllowedDerivedInputs(),
execRoot,
artifactResolver,
siblingRepositoryLayout);
}

@VisibleForTesting
Expand All @@ -1584,22 +1579,16 @@ public NestedSet<Artifact> discoverInputsFromDotdFiles(
boolean siblingRepositoryLayout)
throws ActionExecutionException {
Preconditions.checkNotNull(getDotdFile(), "Trying to scan .d file which is unset");
HeaderDiscovery.Builder discoveryBuilder =
new HeaderDiscovery.Builder()
.setAction(this)
.setSourceFile(getSourceFile())
.setDependencies(
processDepset(actionExecutionContext, execRoot, dotDContents).getDependencies())
.setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot))
.setAllowedDerivedInputs(getAllowedDerivedInputs());

if (needsIncludeValidation) {
discoveryBuilder.shouldValidateInclusions();
}

return discoveryBuilder
.build()
.discoverInputsFromDependencies(execRoot, artifactResolver, siblingRepositoryLayout);
return HeaderDiscovery.discoverInputsFromDependencies(
this,
getSourceFile(),
needsIncludeValidation,
processDepset(actionExecutionContext, execRoot, dotDContents).getDependencies(),
getPermittedSystemIncludePrefixes(execRoot),
getAllowedDerivedInputs(),
execRoot,
artifactResolver,
siblingRepositoryLayout);
}

public DependencySet processDepset(
Expand Down Expand Up @@ -1833,7 +1822,7 @@ private final class CppCompileActionContinuation extends ActionContinuationOrRes
private final ShowIncludesFilter showIncludesFilterForStderr;
private final SpawnContinuation spawnContinuation;

public CppCompileActionContinuation(
CppCompileActionContinuation(
ActionExecutionContext actionExecutionContext,
ActionExecutionContext spawnExecutionContext,
ShowIncludesFilter showIncludesFilterForStdout,
Expand Down
Loading

0 comments on commit a617b5a

Please sign in to comment.