introduce hidden flag to configure bytecode optimizers

--
PiperOrigin-RevId: 151170448
MOS_MIGRATED_REVID=151170448

9 files changed, 166 insertions, 42 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 1fc05d8034..5285ee9f07 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -85,6 +85,7 @@ import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
@@ -279,6 +280,38 @@ public final class BuildConfiguration {
     }
   }
 
+  /** Flag converter for a map of unique keys with optional labels as values. */
+  public static class LabelMapConverter implements Converter<Map<String, Label>> {
+    @Override
+    public Map<String, Label> convert(String input) throws OptionsParsingException {
+      // Use LinkedHashMap so we can report duplicate keys more easily while preserving order
+      Map<String, Label> result = new LinkedHashMap<>();
+      for (String entry : Splitter.on(",").omitEmptyStrings().trimResults().split(input)) {
+        String key;
+        Label label;
+        int sepIndex = entry.indexOf('=');
+        if (sepIndex < 0) {
+          key = entry;
+          label = null;
+        } else {
+          key = entry.substring(0, sepIndex);
+          String value = entry.substring(sepIndex + 1);
+          label = value.isEmpty() ? null : convertLabel(value);
+        }
+        if (result.containsKey(key)) {
+          throw new OptionsParsingException("Key '" + key + "' appears twice");
+        }
+        result.put(key, label);
+      }
+      return Collections.unmodifiableMap(result);
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "a comma-separated list of keys optionally followed by '=' and a label";
+    }
+  }
+
   /** TODO(bazel-team): document this */
   public static class PluginOptionConverter implements Converter<Map.Entry<String, String>> {
     @Override
@@ -2131,7 +2164,7 @@ public final class BuildConfiguration {
   public List<Label> getPlugins() {
     return options.pluginList;
   }
-  
+
   /**
    * Returns the configuration-dependent string for this configuration. This is also the name of the
    * configuration's base output directory unless {@link Options#outputDirectoryName} overrides it.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index a92335518c..8751696579 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -130,7 +130,7 @@ public final class Attribute implements Comparable<Attribute> {
 
   /** A RuleAspect that just wraps a pre-existing Aspect that doesn't vary with the Rule. */
   private static class PredefinedRuleAspect extends RuleAspect<AspectClass> {
-    private Aspect aspect;
+    private final Aspect aspect;
 
     public PredefinedRuleAspect(Aspect aspect) {
       super(aspect.getAspectClass(), null);
@@ -438,7 +438,7 @@ public final class Attribute implements Comparable<Attribute> {
    * already undocumented based on its name cannot be marked as undocumented.
    */
   public static class Builder <TYPE> {
-    private String name;
+    private final String name;
     private final Type<TYPE> type;
     private Transition configTransition = ConfigurationTransition.NONE;
     private Predicate<RuleClass> allowedRuleClassesForLabels = Predicates.alwaysTrue();
@@ -1662,8 +1662,8 @@ public final class Attribute implements Comparable<Attribute> {
     private final ImmutableList<Label> labels;
     private final ImmutableSet<Class<?>> requiredConfigurationFragments;
 
-    public LateBoundLabelList() {
-      this(ImmutableList.<Label>of());
+    public LateBoundLabelList(Class<?>... requiredConfigurationFragments) {
+      this(ImmutableList.<Label>of(), requiredConfigurationFragments);
     }
 
     public LateBoundLabelList(List<Label> labels, Class<?>... requiredConfigurationFragments) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
index 32e2aa5b70..040f60635e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
@@ -1073,7 +1073,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
         ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_PROGUARD_SEEDS);
     Artifact proguardUsage =
         ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_PROGUARD_USAGE);
-    ProguardOutput result = ProguardHelper.createProguardAction(
+    ProguardOutput result = ProguardHelper.createOptimizationActions(
         ruleContext,
         sdk.getProguard(),
         deployJarArtifact,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
index 7b345a16af..5fc82a9535 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
@@ -790,6 +790,8 @@ public final class AndroidRuleClasses {
                   .value(TriState.AUTO)
                   .undocumented("No-op, soon to be removed"))
           .add(attr(":extra_proguard_specs", LABEL_LIST).value(JavaSemantics.EXTRA_PROGUARD_SPECS))
+          .add(attr(":bytecode_optimizers", LABEL_LIST)
+              .cfg(HOST).value(JavaSemantics.BYTECODE_OPTIMIZERS))
           .add(attr("rewrite_dexes_with_rex", BOOLEAN).value(false).undocumented("experimental"))
           /*
           File to be used as a package map for Rex tool that keeps the assignment of classes to
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
index c572dc0590..748258cd56 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
@@ -15,7 +15,9 @@ package com.google.devtools.build.lib.rules.java;
 
 import static com.google.common.base.Preconditions.checkArgument;
 
+import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMap.Builder;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.StrictDepsMode;
@@ -31,6 +33,7 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
 import com.google.devtools.common.options.TriState;
 import java.util.List;
+import java.util.Map;
 import javax.annotation.Nullable;
 
 /** A java compiler configuration containing the flags required for compilation. */
@@ -141,6 +144,7 @@ public final class JavaConfiguration extends Fragment {
   private final TriState bundleTranslations;
   private final ImmutableList<Label> translationTargets;
   private final JavaOptimizationMode javaOptimizationMode;
+  private final ImmutableMap<String, Optional<Label>> bytecodeOptimizers;
   private final Label javaToolchain;
 
   // TODO(dmarting): remove once we have a proper solution for #2539
@@ -185,6 +189,16 @@ public final class JavaConfiguration extends Fragment {
       }
     }
     this.translationTargets = translationsBuilder.build();
+
+    ImmutableMap.Builder<String, Optional<Label>> optimizersBuilder = ImmutableMap.builder();
+    for (Map.Entry<String, Label> optimizer : javaOptions.bytecodeOptimizers.entrySet()) {
+      String mnemonic = optimizer.getKey();
+      if (optimizer.getValue() == null && !"Proguard".equals(mnemonic)) {
+        throw new InvalidConfigurationException("Must supply label for optimizer " + mnemonic);
+      }
+      optimizersBuilder.put(mnemonic, Optional.fromNullable(optimizer.getValue()));
+    }
+    this.bytecodeOptimizers = optimizersBuilder.build();
   }
 
   @SkylarkCallable(name = "default_javac_flags", structField = true,
@@ -327,6 +341,13 @@ public final class JavaConfiguration extends Fragment {
   }
 
   /**
+   * Returns ordered list of optimizers to run.
+   */
+  public ImmutableMap<String, Optional<Label>> getBytecodeOptimizers() {
+    return bytecodeOptimizers;
+  }
+
+  /**
    * Returns true if java_test in Bazel should behave in legacy mode that existed before we
    * open-sourced our test runner.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
index a7b7086ee8..b489c21022 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
@@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.LabelConverter;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration.LabelMapConverter;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.StrictDepsConverter;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.StrictDepsMode;
 import com.google.devtools.build.lib.analysis.config.DefaultsPackage;
@@ -270,6 +271,18 @@ public class JavaOptions extends FragmentOptions {
           + "using this option only has an effect when Proguard is used anyway.")
   public List<Label> extraProguardSpecs;
 
+  /**
+   * Comma-separated list of Mnemonic=label pairs of optimizers to run in the given order, treating
+   * {@code Proguard} specially by substituting in the relevant Proguard binary automatically.
+   * All optimizers must understand the same flags as Proguard.
+   */
+  @Option(name = "experimental_bytecode_optimizers",
+      defaultValue = "Proguard",
+      converter = LabelMapConverter.class,
+      category = "undocumented",
+      help = "Do not use.")
+  public Map<String, Label> bytecodeOptimizers;
+
   @Option(name = "translations",
       defaultValue = "auto",
       category = "semantics",
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
index 9c3d443119..77cdbbe8ad 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
@@ -15,8 +15,10 @@
 package com.google.devtools.build.lib.rules.java;
 
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 import static com.google.devtools.build.lib.packages.ImplicitOutputsFunction.fromTemplates;
 
+import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -38,6 +40,7 @@ import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SafeImplic
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.rules.java.DeployArchiveBuilder.Compression;
 import com.google.devtools.build.lib.rules.java.JavaCompilationArgs.ClasspathType;
+import com.google.devtools.build.lib.rules.java.JavaConfiguration.JavaOptimizationMode;
 import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -200,6 +203,29 @@ public interface JavaSemantics {
         }
       };
 
+  LateBoundLabelList<BuildConfiguration> BYTECODE_OPTIMIZERS =
+      new LateBoundLabelList<BuildConfiguration>(JavaConfiguration.class) {
+        @Override
+        public List<Label> resolve(
+            Rule rule, AttributeMap attributes, BuildConfiguration configuration) {
+          // Use a modicum of smarts to avoid implicit dependencies where we don't need them.
+          JavaOptimizationMode optMode =
+              configuration.getFragment(JavaConfiguration.class).getJavaOptimizationMode();
+          boolean hasProguardSpecs = attributes.has("proguard_specs")
+              && !attributes.get("proguard_specs", LABEL_LIST).isEmpty();
+          if (optMode == JavaOptimizationMode.NOOP
+              || (optMode == JavaOptimizationMode.LEGACY && !hasProguardSpecs)) {
+            return ImmutableList.<Label>of();
+          }
+          return ImmutableList.copyOf(
+              Optional.presentInstances(
+                  configuration
+                      .getFragment(JavaConfiguration.class)
+                      .getBytecodeOptimizers()
+                      .values()));
+        }
+      };
+
   String IJAR_LABEL = "//tools/defaults:ijar";
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/ProguardHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/ProguardHelper.java
index 49c13824ac..b6445e304f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/ProguardHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/ProguardHelper.java
@@ -14,8 +14,10 @@
 package com.google.devtools.build.lib.rules.java;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSortedSet;
@@ -24,15 +26,18 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
+import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction.Builder;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.rules.java.JavaConfiguration.JavaOptimizationMode;
 import com.google.devtools.build.lib.syntax.Type;
+import java.util.Map;
 import javax.annotation.Nullable;
 
 /**
@@ -186,7 +191,7 @@ public abstract class ProguardHelper {
       proguardOutputMap =
           ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_PROGUARD_MAP);
     }
-    return createProguardAction(
+    return createOptimizationActions(
         ruleContext,
         proguard,
         singleJar,
@@ -364,15 +369,13 @@ public abstract class ProguardHelper {
    * @param proguardSpecs Proguard specification files to pass to Proguard
    * @param proguardMapping optional mapping file for Proguard to apply
    * @param libraryJars any other Jar files that the {@code programJar} will run against
-   * @param mappingRequested whether to ask Proguard to output a mapping file (a mapping will be
-   *        produced anyway if --java_optimization_mode includes obfuscation)
    * @param optimizationPasses if not null specifies to break proguard up into multiple passes with
    *        the given number of optimization passes.
    * @param proguardOutputMap mapping generated by Proguard if requested. could be null.
    * @param useSingleJarForProguardLibraryJars whether to combine all library jars into a single jar
    *        file before processing with Proguard
    */
-  public static ProguardOutput createProguardAction(RuleContext ruleContext,
+  public static ProguardOutput createOptimizationActions(RuleContext ruleContext,
       FilesToRunProvider proguard,
       Artifact programJar,
       ImmutableList<Artifact> proguardSpecs,
@@ -409,7 +412,7 @@ public abstract class ProguardHelper {
 
     if (optimizationPasses == null) {
       // Run proguard as a single step.
-      Builder builder = makeBuilder(
+      SpawnAction.Builder builder = defaultAction(
           proguard,
           programJar,
           proguardSpecs,
@@ -431,7 +434,7 @@ public abstract class ProguardHelper {
       Artifact lastStageOutput = getProguardTempArtifact(
           ruleContext, optMode.name().toLowerCase(), "proguard_preoptimization.jar");
       ruleContext.registerAction(
-          makeBuilder(
+          defaultAction(
               proguard,
               programJar,
               proguardSpecs,
@@ -450,34 +453,55 @@ public abstract class ProguardHelper {
               .addOutputArgument(lastStageOutput)
               .build(ruleContext));
 
-      for (int i = 0; i < optimizationPasses; i++) {
-        Artifact optimizationOutput = getProguardTempArtifact(
-            ruleContext, optMode.name().toLowerCase(), "proguard_optimization_" + (i + 1) + ".jar");
-        ruleContext.registerAction(
-            makeBuilder(
-                proguard,
-                programJar,
-                proguardSpecs,
-                proguardMapping,
-                libraryJars,
-                output.getOutputJar(),
-                /* proguardOutputMap */ null,
-                /* proguardOutputProtoMap */ null,
-                /* proguardSeeds */ null,
-                /* proguardUsage */ null,
-                /* constantStringObfuscatedMapping */ null,
-                /* proguardConfigOutput */ null)
-                .setProgressMessage("Trimming binary with Proguard: Optimization Pass " + (i + 1))
-                .addArgument("-runtype OPTIMIZATION")
-                .addArgument("-laststageoutput")
-                .addInputArgument(lastStageOutput)
-                .addArgument("-nextstageoutput")
-                .addOutputArgument(optimizationOutput)
-                .build(ruleContext));
-        lastStageOutput = optimizationOutput;
+      for (int i = 1; i <= optimizationPasses; i++) {
+        // Run configured optimizers in order in each pass
+        for (Map.Entry<String, Optional<Label>> optimizer :
+            getBytecodeOptimizers(ruleContext).entrySet()) {
+          String mnemonic = optimizer.getKey();
+          Optional<Label> target = optimizer.getValue();
+          FilesToRunProvider executable = null;
+          if (target.isPresent()) {
+            for (TransitiveInfoCollection optimizerDep :
+                ruleContext.getPrerequisites(":bytecode_optimizers", Mode.HOST)) {
+              if (optimizerDep.getLabel().equals(target.get())) {
+                executable = optimizerDep.getProvider(FilesToRunProvider.class);
+                break;
+              }
+            }
+          } else {
+            checkState("Proguard".equals(mnemonic), "Need label to run %s", mnemonic);
+            executable = proguard;
+          }
+          Artifact optimizationOutput = getProguardTempArtifact(
+              ruleContext, optMode.name().toLowerCase(), mnemonic + "_optimization_" + i + ".jar");
+          ruleContext.registerAction(
+              defaultAction(
+                  checkNotNull(executable, "couldn't find optimizer %s", optimizer),
+                  programJar,
+                  proguardSpecs,
+                  proguardMapping,
+                  libraryJars,
+                  output.getOutputJar(),
+                  /* proguardOutputMap */ null,
+                  /* proguardOutputProtoMap */ null,
+                  /* proguardSeeds */ null,
+                  /* proguardUsage */ null,
+                  /* constantStringObfuscatedMapping */ null,
+                  /* proguardConfigOutput */ null)
+                  .setProgressMessage(
+                      "Trimming binary with " + mnemonic + ": Optimization Pass " + i)
+                  .setMnemonic(mnemonic)
+                  .addArgument("-runtype OPTIMIZATION")
+                  .addArgument("-laststageoutput")
+                  .addInputArgument(lastStageOutput)
+                  .addArgument("-nextstageoutput")
+                  .addOutputArgument(optimizationOutput)
+                  .build(ruleContext));
+          lastStageOutput = optimizationOutput;
+        }
       }
 
-      Builder builder = makeBuilder(
+      SpawnAction.Builder builder = defaultAction(
           proguard,
           programJar,
           proguardSpecs,
@@ -502,8 +526,8 @@ public abstract class ProguardHelper {
     return output;
   }
 
-  private static Builder makeBuilder(
-      FilesToRunProvider proguard,
+  private static SpawnAction.Builder defaultAction(
+      FilesToRunProvider executable,
       Artifact programJar,
       ImmutableList<Artifact> proguardSpecs,
       @Nullable Artifact proguardMapping,
@@ -519,7 +543,7 @@ public abstract class ProguardHelper {
     Builder builder = new SpawnAction.Builder()
         .addInputs(libraryJars)
         .addInputs(proguardSpecs)
-        .setExecutable(proguard)
+        .setExecutable(executable)
         .setMnemonic("Proguard")
         .addArgument("-forceprocessing")
         .addArgument("-injars")
@@ -611,4 +635,9 @@ public abstract class ProguardHelper {
     return ruleContext.getConfiguration().getFragment(JavaConfiguration.class)
         .getJavaOptimizationMode();
   }
+
+  private static Map<String, Optional<Label>> getBytecodeOptimizers(RuleContext ruleContext) {
+    return ruleContext.getConfiguration().getFragment(JavaConfiguration.class)
+        .getBytecodeOptimizers();
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
index 62c30bc67f..5cdedf9828 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
@@ -16,7 +16,7 @@ package com.google.devtools.build.lib.rules.android;
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.getFirstArtifactEndingWith;
-import static junit.framework.TestCase.assertTrue;
+import static junit.framework.Assert.assertTrue;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;