diff options
author | Liam Miller-Cushon <cushon@google.com> | 2016-06-22 08:24:56 +0000 |
---|---|---|
committer | Philipp Wollermann <philwo@google.com> | 2016-06-22 10:48:55 +0000 |
commit | 53c8f94cfb2ac7d41c8815e40e3d9d834789142b (patch) | |
tree | 194973b5bc23f9ed2f04b364a5aa15b1394b16f1 | |
parent | 182639f13adca82053ef037f57b8a639b725ded3 (diff) |
Rollback of commit 611e7cd47de47fd7cc7e08a260d6640803aafd9f.
*** Reason for rollback ***
Introduced classpath ordering bug
*** Original change description ***
Fix analysis performance regression caused by header compilation
When building --direct_dependency/--indirect_dependency args, use a set for the
direct jars to avoid an O(n) contains check, and make the CustomMultiArgv class
static to avoid a memory leak. Also avoid flattening the NestedSet of direct
jars for as long as possible.
--
MOS_MIGRATED_REVID=125541563
6 files changed, 103 insertions, 93 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java index edc438dbb3..870cee733a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java @@ -375,9 +375,8 @@ public class AndroidCommon { ImmutableList.Builder<Artifact> jarsProducedForRuntime) throws InterruptedException { compileResourceJar(javaSemantics, resourcesJar); // Add the compiled resource jar to the classpath of the main compilation. - NestedSet<Artifact> directJars = NestedSetBuilder.create(Order.STABLE_ORDER, resourceClassJar); - attributes.addDirectJars(directJars); - attributes.addCompileTimeClassPathEntries(directJars); + attributes.addDirectJars(ImmutableList.of(resourceClassJar)); + attributes.addDirectCompileTimeClassPathEntries(ImmutableList.of(resourceClassJar)); // Add the compiled resource jar to the classpath of consuming targets. artifactsBuilder.addCompileTimeJar(resourceClassJar); // Combined resource constants needs to come even before our own classes that may contain diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java index 84fd903601..e4f654b941 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java @@ -157,7 +157,7 @@ public final class JavaCompilationHelper extends BaseJavaCompilationHelper { builder.addProcessorPaths(attributes.getProcessorPath()); builder.addProcessorNames(attributes.getProcessorNames()); builder.setStrictJavaDeps(attributes.getStrictJavaDeps()); - builder.setDirectJars(attributes.getDirectJars()); + builder.addDirectJars(attributes.getDirectJars()); builder.addCompileTimeDependencyArtifacts(attributes.getCompileTimeDependencyArtifacts()); builder.setRuleKind(attributes.getRuleKind()); builder.setTargetLabel(attributes.getTargetLabel()); @@ -279,7 +279,7 @@ public final class JavaCompilationHelper extends BaseJavaCompilationHelper { builder.setOutputDepsProto(headerDeps); builder.setStrictJavaDeps(attributes.getStrictJavaDeps()); builder.addCompileTimeDependencyArtifacts(attributes.getCompileTimeDependencyArtifacts()); - builder.setDirectJars(attributes.getDirectJars()); + builder.addDirectJars(attributes.getDirectJars()); builder.setRuleKind(attributes.getRuleKind()); builder.setTargetLabel(attributes.getTargetLabel()); builder.setJavaBaseInputs(getHostJavabaseInputsNonStatic(ruleContext)); @@ -521,11 +521,10 @@ public final class JavaCompilationHelper extends BaseJavaCompilationHelper { return jar; } - private void addArgsAndJarsToAttributes( - JavaCompilationArgs args, NestedSet<Artifact> directJars) { + private void addArgsAndJarsToAttributes(JavaCompilationArgs args, Iterable<Artifact> directJars) { // Can only be non-null when isStrict() returns true. if (directJars != null) { - attributes.addCompileTimeClassPathEntries(directJars); + attributes.addDirectCompileTimeClassPathEntries(directJars); attributes.addDirectJars(directJars); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index c9abbda252..5c3d36d41e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.rules.java; -import static com.google.devtools.build.lib.util.Preconditions.checkNotNull; import static java.nio.charset.StandardCharsets.ISO_8859_1; import com.google.common.annotations.VisibleForTesting; @@ -69,7 +68,6 @@ import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.Set; /** * Action that represents a Java compilation. @@ -146,8 +144,10 @@ public final class JavaCompileAction extends AbstractAction { */ private final ImmutableList<String> javacOpts; - /** The subset of classpath jars provided by direct dependencies. */ - private final NestedSet<Artifact> directJars; + /** + * The subset of classpath jars provided by direct dependencies. + */ + private final ImmutableList<Artifact> directJars; /** * The level of strict dependency checks (off, warnings, or errors). @@ -163,12 +163,12 @@ public final class JavaCompileAction extends AbstractAction { * Constructs an action to compile a set of Java source files to class files. * * @param owner the action owner, typically a java_* RuleConfiguredTarget. - * @param baseInputs the set of the input artifacts of the compile action without the parameter - * file action; + * @param baseInputs the set of the input artifacts of the compile action + * without the parameter file action; * @param outputs the outputs of the action - * @param javaCompileCommandLine the command line for the java library builder - it's actually - * written to the parameter file, but other parts (for example, ide_build_info) need access to - * the data + * @param javaCompileCommandLine the command line for the java library + * builder - it's actually written to the parameter file, but other + * parts (for example, ide_build_info) need access to the data * @param commandLine the actual invocation command line */ private JavaCompileAction( @@ -191,7 +191,7 @@ public final class JavaCompileAction extends AbstractAction { Collection<Artifact> sourceJars, Collection<Artifact> sourceFiles, List<String> javacOpts, - NestedSet<Artifact> directJars, + Collection<Artifact> directJars, BuildConfiguration.StrictDepsMode strictJavaDeps, Collection<Artifact> compileTimeDependencyArtifacts) { super( @@ -213,7 +213,7 @@ public final class JavaCompileAction extends AbstractAction { this.javaCompileCommandLine = javaCompileCommandLine; this.commandLine = commandLine; - this.classDirectory = checkNotNull(classDirectory); + this.classDirectory = Preconditions.checkNotNull(classDirectory); this.outputJar = outputJar; this.classpathEntries = classpathEntries; this.bootclasspathEntries = ImmutableList.copyOf(bootclasspathEntries); @@ -226,7 +226,7 @@ public final class JavaCompileAction extends AbstractAction { this.sourceJars = ImmutableList.copyOf(sourceJars); this.sourceFiles = ImmutableList.copyOf(sourceFiles); this.javacOpts = ImmutableList.copyOf(javacOpts); - this.directJars = checkNotNull(directJars, "directJars must not be null"); + this.directJars = ImmutableList.copyOf(directJars); this.strictJavaDeps = strictJavaDeps; this.compileTimeDependencyArtifacts = ImmutableList.copyOf(compileTimeDependencyArtifacts); } @@ -292,7 +292,7 @@ public final class JavaCompileAction extends AbstractAction { } @VisibleForTesting - public NestedSet<Artifact> getDirectJars() { + public Collection<Artifact> getDirectJars() { return directJars; } @@ -472,13 +472,13 @@ public final class JavaCompileAction extends AbstractAction { * Creates an instance. * * @param configuration the build configuration, which provides the default options and the path - * to the compiler, etc. + * to the compiler, etc. * @param classDirectory the directory in which generated classfiles are placed relative to the - * exec root + * exec root * @param sourceGenDirectory the directory where source files generated by annotation processors - * should be stored. + * should be stored. * @param tempDirectory a directory in which the library builder can store temporary files - * relative to the exec root + * relative to the exec root * @param outputJar output jar * @param compressJar if true compress the output jar * @param outputDepsProto the proto file capturing dependency information @@ -513,13 +513,13 @@ public final class JavaCompileAction extends AbstractAction { Collection<Artifact> sourceFiles, Collection<Artifact> extdirInputs, List<String> javacOpts, - final NestedSet<Artifact> directJars, + final Collection<Artifact> directJars, BuildConfiguration.StrictDepsMode strictJavaDeps, Collection<Artifact> compileTimeDependencyArtifacts, String ruleKind, Label targetLabel) { - checkNotNull(classDirectory); - checkNotNull(tempDirectory); + Preconditions.checkNotNull(classDirectory); + Preconditions.checkNotNull(tempDirectory); CustomCommandLine.Builder result = CustomCommandLine.builder(); @@ -615,7 +615,12 @@ public final class JavaCompileAction extends AbstractAction { if (strictJavaDeps != BuildConfiguration.StrictDepsMode.OFF) { result.add("--strict_java_deps"); result.add(strictJavaDeps.toString()); - result.add(new JarsToTargetsArgv(classpath, directJars)); + result.add(new CustomMultiArgv() { + @Override + public Iterable<String> argv() { + return addJarsToTargets(classpath, directJars); + } + }); if (configuration.getFragment(JavaConfiguration.class).getReduceJavaClasspath() == JavaClasspathMode.JAVABUILDER) { @@ -668,34 +673,26 @@ public final class JavaCompileAction extends AbstractAction { } /** - * Builds the list of mappings between jars on the classpath and their originating targets names. + * Builds the list of mappings between jars on the classpath and their + * originating targets names. */ - static class JarsToTargetsArgv extends CustomMultiArgv { - private final NestedSet<Artifact> classpath; - private final NestedSet<Artifact> directJars; - - JarsToTargetsArgv(NestedSet<Artifact> classpath, NestedSet<Artifact> directJars) { - this.classpath = classpath; - this.directJars = directJars; - } - - @Override - public Iterable<String> argv() { - Set<Artifact> directJarSet = directJars.toSet(); - ImmutableList.Builder<String> builder = ImmutableList.builder(); - for (Artifact jar : classpath) { - builder.add(directJarSet.contains(jar) ? "--direct_dependency" : "--indirect_dependency"); - builder.add(jar.getExecPathString()); - Label label = getTargetName(jar); - builder.add( - label.getPackageIdentifier().getRepository().isDefault() - || label.getPackageIdentifier().getRepository().isMain() - ? label.toString() - // Escape '@' prefix for .params file. - : "@" + label); - } - return builder.build(); + static ImmutableList<String> addJarsToTargets( + NestedSet<Artifact> classpath, Collection<Artifact> directJars) { + ImmutableList.Builder<String> builder = ImmutableList.builder(); + for (Artifact jar : classpath) { + builder.add(directJars.contains(jar) + ? "--direct_dependency" + : "--indirect_dependency"); + builder.add(jar.getExecPathString()); + Label label = getTargetName(jar); + builder.add( + label.getPackageIdentifier().getRepository().isDefault() + || label.getPackageIdentifier().getRepository().isMain() + ? label.toString() + // Escape '@' prefix for .params file. + : "@" + label); } + return builder.build(); } /** @@ -705,7 +702,7 @@ public final class JavaCompileAction extends AbstractAction { * libraries), there is no generating action, so we just return the jar name in label form. */ private static Label getTargetName(Artifact jar) { - return checkNotNull(jar.getOwner(), jar); + return Preconditions.checkNotNull(jar.getOwner(), jar); } /** @@ -715,8 +712,8 @@ public final class JavaCompileAction extends AbstractAction { Artifact langtoolsJar, ImmutableList<Artifact> instrumentationJars, Artifact paramFile, ImmutableList<String> javaBuilderJvmFlags, String javaBuilderMainClass, String pathDelimiter) { - checkNotNull(langtoolsJar); - checkNotNull(javaBuilderJar); + Preconditions.checkNotNull(langtoolsJar); + Preconditions.checkNotNull(javaBuilderJar); CustomCommandLine.Builder builder = CustomCommandLine.builder() .addPath(javaExecutable) @@ -789,7 +786,7 @@ public final class JavaCompileAction extends AbstractAction { private final Collection<Artifact> translations = new LinkedHashSet<>(); private BuildConfiguration.StrictDepsMode strictJavaDeps = BuildConfiguration.StrictDepsMode.OFF; - private NestedSet<Artifact> directJars = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER); + private final Collection<Artifact> directJars = new ArrayList<>(); private final Collection<Artifact> compileTimeDependencyArtifacts = new ArrayList<>(); private List<String> javacOpts = new ArrayList<>(); private ImmutableList<String> javacJvmOpts = ImmutableList.of(); @@ -858,7 +855,7 @@ public final class JavaCompileAction extends AbstractAction { // Invariant: if strictJavaDeps is OFF, then directJars and // dependencyArtifacts are ignored if (strictJavaDeps == BuildConfiguration.StrictDepsMode.OFF) { - directJars = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER); + directJars.clear(); compileTimeDependencyArtifacts.clear(); } @@ -1047,9 +1044,11 @@ public final class JavaCompileAction extends AbstractAction { return this; } - /** Accumulates the given jar artifacts as being provided by direct dependencies. */ - public Builder setDirectJars(NestedSet<Artifact> directJars) { - this.directJars = checkNotNull(directJars, "directJars must not be null"); + /** + * Accumulates the given jar artifacts as being provided by direct dependencies. + */ + public Builder addDirectJars(Collection<Artifact> directJars) { + this.directJars.addAll(directJars); return this; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java index 895d1b63b5..2dfe4a6ed9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.actions.CommandLine; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; +import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.CustomMultiArgv; import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfiguration.StrictDepsMode; @@ -70,7 +71,7 @@ public class JavaHeaderCompileActionBuilder { @Nullable private Label targetLabel; private PathFragment tempDirectory; private BuildConfiguration.StrictDepsMode strictJavaDeps = BuildConfiguration.StrictDepsMode.OFF; - private NestedSet<Artifact> directJars = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER); + private List<Artifact> directJars = new ArrayList<>(); private List<Artifact> compileTimeDependencyArtifacts = new ArrayList<>(); private ImmutableList<String> javacOpts; private List<Artifact> processorPath = new ArrayList<>(); @@ -91,9 +92,9 @@ public class JavaHeaderCompileActionBuilder { } /** Sets the direct dependency artifacts. */ - public JavaHeaderCompileActionBuilder setDirectJars(NestedSet<Artifact> directJars) { + public JavaHeaderCompileActionBuilder addDirectJars(Collection<Artifact> directJars) { checkNotNull(directJars, "directJars must not be null"); - this.directJars = directJars; + this.directJars.addAll(directJars); return this; } @@ -236,13 +237,6 @@ public class JavaHeaderCompileActionBuilder { checkNotNull(processorPath, "processorPath must not be null"); checkNotNull(processorNames, "processorNames must not be null"); - // Invariant: if strictJavaDeps is OFF, then directJars and - // dependencyArtifacts are ignored - if (strictJavaDeps == BuildConfiguration.StrictDepsMode.OFF) { - directJars = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER); - compileTimeDependencyArtifacts.clear(); - } - SpawnAction.Builder builder = new SpawnAction.Builder(); builder.addOutput(outputJar); @@ -259,7 +253,7 @@ public class JavaHeaderCompileActionBuilder { builder.addInputs(processorPath); builder.addInputs(sourceJars); builder.addInputs(sourceFiles); - builder.addTransitiveInputs(directJars); + builder.addInputs(directJars); builder.addInputs(compileTimeDependencyArtifacts); builder.addTool(javacJar); @@ -337,7 +331,13 @@ public class JavaHeaderCompileActionBuilder { } if (strictJavaDeps != BuildConfiguration.StrictDepsMode.OFF) { - result.add(new JavaCompileAction.JarsToTargetsArgv(classpathEntries, directJars)); + result.add( + new CustomMultiArgv() { + @Override + public Iterable<String> argv() { + return JavaCompileAction.addJarsToTargets(classpathEntries, directJars); + } + }); if (!compileTimeDependencyArtifacts.isEmpty()) { result.addExecPaths("--deps_artifacts", compileTimeDependencyArtifacts); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibraryHelper.java index bd4409ddc2..4112128e65 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibraryHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibraryHelper.java @@ -174,7 +174,7 @@ public final class JavaLibraryHelper { if (isStrict()) { directJars = getNonRecursiveCompileTimeJarsFromDeps(); if (directJars != null) { - attributes.addCompileTimeClassPathEntries(directJars); + attributes.addDirectCompileTimeClassPathEntries(directJars); attributes.addDirectJars(directJars); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaTargetAttributes.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaTargetAttributes.java index 415cb12139..3a28c203be 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaTargetAttributes.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaTargetAttributes.java @@ -83,7 +83,7 @@ public class JavaTargetAttributes { private BuildConfiguration.StrictDepsMode strictJavaDeps = BuildConfiguration.StrictDepsMode.OFF; - private final NestedSetBuilder<Artifact> directJars = NestedSetBuilder.naiveLinkOrder(); + private final List<Artifact> directJars = new ArrayList<>(); private final List<Artifact> compileTimeDependencyArtifacts = new ArrayList<>(); private String ruleKind; private Label targetLabel; @@ -174,6 +174,15 @@ public class JavaTargetAttributes { return this; } + public Builder addDirectCompileTimeClassPathEntries(Iterable<Artifact> entries) { + Preconditions.checkArgument(!built); + // The other version is preferred as it is more memory-efficient. + for (Artifact classPathEntry : entries) { + compileTimeClassPath.add(classPathEntry); + } + return this; + } + public Builder setRuleKind(String ruleKind) { Preconditions.checkArgument(!built); this.ruleKind = ruleKind; @@ -219,21 +228,23 @@ public class JavaTargetAttributes { } /** - * In tandem with strictJavaDeps, directJars represents a subset of the compile-time, classpath - * jars that were provided by direct dependencies. When strictJavaDeps is OFF, there is no need - * to provide directJars, and no extra information is passed to javac. When strictJavaDeps is - * set to WARN or ERROR, the compiler command line will include extra flags to indicate the - * warning/error policy and to map the classpath jars to direct or transitive dependencies, - * using the information in directJars. The extra flags are formatted like this (same for - * --indirect_dependency): <pre> + * In tandem with strictJavaDeps, directJars represents a subset of the + * compile-time, classpath jars that were provided by direct dependencies. + * When strictJavaDeps is OFF, there is no need to provide directJars, and + * no extra information is passed to javac. When strictJavaDeps is set to + * WARN or ERROR, the compiler command line will include extra flags to + * indicate the warning/error policy and to map the classpath jars to direct + * or transitive dependencies, using the information in directJars. The extra + * flags are formatted like this (same for --indirect_dependency): * --direct_dependency * foo/bar/lib.jar * //java/com/google/foo:bar - * </pre> + * + * @param directJars */ - public Builder addDirectJars(NestedSet<Artifact> directJars) { + public Builder addDirectJars(Iterable<Artifact> directJars) { Preconditions.checkArgument(!built); - this.directJars.addTransitive(directJars); + Iterables.addAll(this.directJars, directJars); return this; } @@ -328,7 +339,7 @@ public class JavaTargetAttributes { messages, sourceJars, classPathResources, - directJars.build(), + directJars, compileTimeDependencyArtifacts, ruleKind, targetLabel, @@ -380,7 +391,7 @@ public class JavaTargetAttributes { private final ImmutableList<Artifact> classPathResources; - private final NestedSet<Artifact> directJars; + private final ImmutableList<Artifact> directJars; private final ImmutableList<Artifact> compileTimeDependencyArtifacts; private final String ruleKind; private final Label targetLabel; @@ -388,7 +399,9 @@ public class JavaTargetAttributes { private final NestedSet<Artifact> excludedArtifacts; private final BuildConfiguration.StrictDepsMode strictJavaDeps; - /** Constructor of JavaTargetAttributes. */ + /** + * Constructor of JavaTargetAttributes. + */ private JavaTargetAttributes( Set<Artifact> sourceFiles, Set<Artifact> compileTimeJarFiles, @@ -402,7 +415,7 @@ public class JavaTargetAttributes { List<Artifact> messages, List<Artifact> sourceJars, List<Artifact> classPathResources, - NestedSet<Artifact> directJars, + List<Artifact> directJars, List<Artifact> compileTimeDependencyArtifacts, String ruleKind, Label targetLabel, @@ -420,7 +433,7 @@ public class JavaTargetAttributes { this.messages = ImmutableList.copyOf(messages); this.sourceJars = ImmutableList.copyOf(sourceJars); this.classPathResources = ImmutableList.copyOf(classPathResources); - this.directJars = directJars; + this.directJars = ImmutableList.copyOf(directJars); this.compileTimeDependencyArtifacts = ImmutableList.copyOf(compileTimeDependencyArtifacts); this.ruleKind = ruleKind; this.targetLabel = targetLabel; @@ -428,7 +441,7 @@ public class JavaTargetAttributes { this.strictJavaDeps = strictJavaDeps; } - public NestedSet<Artifact> getDirectJars() { + public List<Artifact> getDirectJars() { return directJars; } |