Improve memory footprint of CcToolchainFeatures.Variables.

--
MOS_MIGRATED_REVID=140131142

4 files changed, 43 insertions, 34 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
index 694f4e6e53..83bcf05325 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
@@ -29,7 +29,6 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
 import com.google.common.collect.Sets.SetView;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.VariableValue;
 import com.google.devtools.build.lib.util.Pair;
@@ -972,17 +971,13 @@ public class CcToolchainFeatures implements Serializable {
     @Immutable
     private static final class StringSequence implements VariableValue {
 
-      private final Iterable<String> values;
+      private final ImmutableList<String> values;
 
       public StringSequence(ImmutableList<String> values) {
         Preconditions.checkNotNull(values, "Cannot create StringSequence from null");
         this.values = values;
       }
 
-      StringSequence(NestedSet<String> values) {
-        this.values = values;
-      }
-
       @Override
       public Iterable<? extends VariableValue> getSequenceValue(String variableName) {
         final ImmutableList.Builder<VariableValue> sequences = ImmutableList.builder();
@@ -1149,30 +1144,27 @@ public class CcToolchainFeatures implements Serializable {
      */
     public static class Builder {
       private final Map<String, VariableValue> variablesMap = new LinkedHashMap<>();
+      private final Map<String, String> stringVariablesMap = new LinkedHashMap<>();
 
       /** Add a variable that expands {@code name} to {@code value}. */
       public Builder addStringVariable(String name, String value) {
         Preconditions.checkArgument(
             !variablesMap.containsKey(name), "Cannot overwrite variable '%s'", name);
+        Preconditions.checkArgument(
+            !stringVariablesMap.containsKey(name), "Cannot overwrite variable '%s'", name);
         Preconditions.checkNotNull(
             value, "Cannot set null as a value for variable '%s'", name);
-        variablesMap.put(name, new StringValue(value));
+        stringVariablesMap.put(name, value);
         return this;
       }
 
       /** Add a sequence variable that expands {@code name} to {@code values}. */
-      public Builder addStringSequenceVariable(String name, ImmutableList<String> values) {
+      public Builder addStringSequenceVariable(String name, Iterable<String> values) {
         Preconditions.checkArgument(
             !variablesMap.containsKey(name), "Cannot overwrite variable '%s'", name);
-        variablesMap.put(name, new StringSequence(values));
-        return this;
-      }
-
-      /** Add a sequence variable that expands {@code name} to {@code values}. */
-      public Builder addStringSequenceVariable(String name, NestedSet<String> values) {
-        Preconditions.checkArgument(
-            !variablesMap.containsKey(name), "Cannot overwrite variable '%s'", name);
-        variablesMap.put(name, new StringSequence(values));
+        ImmutableList.Builder<String> builder = ImmutableList.builder();
+        builder.addAll(values);
+        variablesMap.put(name, new StringSequence(builder.build()));
         return this;
       }
 
@@ -1196,8 +1188,10 @@ public class CcToolchainFeatures implements Serializable {
         for (String name : variables.keySet()) {
           Preconditions.checkArgument(
               !variablesMap.containsKey(name), "Cannot overwrite variable '%s'", name);
-          variablesMap.put(name, new StringValue(variables.get(name)));
+          Preconditions.checkArgument(
+              !stringVariablesMap.containsKey(name), "Cannot overwrite variable '%s'", name);
         }
+        stringVariablesMap.putAll(variables);
         return this;
       }
 
@@ -1205,9 +1199,15 @@ public class CcToolchainFeatures implements Serializable {
       public Builder addAll(Variables variables) {
         SetView<String> intersection =
             Sets.intersection(variables.variablesMap.keySet(), variablesMap.keySet());
+        SetView<String> stringIntersection =
+            Sets.intersection(variables.stringVariablesMap.keySet(), stringVariablesMap.keySet());
         Preconditions.checkArgument(
             intersection.isEmpty(), "Cannot overwrite existing variables: %s", intersection);
+        Preconditions.checkArgument(
+            stringIntersection.isEmpty(),
+            "Cannot overwrite existing variables: %s", stringIntersection);
         this.variablesMap.putAll(variables.variablesMap);
+        this.stringVariablesMap.putAll(variables.stringVariablesMap);
         return this;
       }
 
@@ -1218,6 +1218,7 @@ public class CcToolchainFeatures implements Serializable {
        */
       Builder addAndOverwriteAll(Variables overwrittenVariables) {
         this.variablesMap.putAll(overwrittenVariables.variablesMap);
+        this.stringVariablesMap.putAll(overwrittenVariables.stringVariablesMap);
         return this;
       }
 
@@ -1225,7 +1226,8 @@ public class CcToolchainFeatures implements Serializable {
        * @return a new {@Variables} object.
        */
       Variables build() {
-        return new Variables(ImmutableMap.copyOf(variablesMap));
+        return new Variables(
+            ImmutableMap.copyOf(variablesMap), ImmutableMap.copyOf(stringVariablesMap));
       }
     }
     
@@ -1238,10 +1240,14 @@ public class CcToolchainFeatures implements Serializable {
     }
     
     private final ImmutableMap<String, VariableValue> variablesMap;
+    private final ImmutableMap<String, String> stringVariablesMap;
     private final Variables parent;
 
-    private Variables(ImmutableMap<String, VariableValue> variablesMap) {
+    private Variables(
+        ImmutableMap<String, VariableValue> variablesMap,
+        ImmutableMap<String, String> stringVariablesMap) {
       this.variablesMap = variablesMap;
+      this.stringVariablesMap = stringVariablesMap;
       this.parent = null;
     }
 
@@ -1251,6 +1257,7 @@ public class CcToolchainFeatures implements Serializable {
      */
     private Variables(Variables parent, String name, VariableValue value) {
       this.variablesMap = ImmutableMap.of(name, value);
+      this.stringVariablesMap = ImmutableMap.of();
       this.parent = parent;
     }
 
@@ -1302,6 +1309,9 @@ public class CcToolchainFeatures implements Serializable {
       if (variablesMap.containsKey(name)) {
         return Pair.of(variablesMap.get(name), null);
       }
+      if (stringVariablesMap.containsKey(name)) {
+        return Pair.<VariableValue, String>of(new StringValue(stringVariablesMap.get(name)), null);
+      }
 
       if (parent != null) {
         return parent.getNonStructuredVariable(name);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
index 032624bdd6..a368cbbe36 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -1217,8 +1217,8 @@ public class CppLinkActionBuilder {
       return rpathEntries;
     }
 
-    public ImmutableList<String> getLibopts() {
-      return libopts.asList();
+    public ImmutableSet<String> getLibopts() {
+      return libopts;
     }
 
     public ImmutableList<String> getLinkerInputParams() {
@@ -1300,7 +1300,7 @@ public class CppLinkActionBuilder {
       }
 
       buildVariables.addStringSequenceVariable(
-          LINKSTAMP_PATHS_VARIABLE, linkstampPaths.build().asList());
+          LINKSTAMP_PATHS_VARIABLE, linkstampPaths.build());
 
       // pic
       boolean forcePic = cppConfiguration.forcePic();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index d1eadd4e1e..8aa4ecc31c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -342,19 +342,19 @@ public final class CppModel {
   }
 
   /** Get the safe path strings for a list of paths to use in the build variables. */
-  private ImmutableList<String> getSafePathStrings(Collection<PathFragment> paths) {
+  private ImmutableSet<String> getSafePathStrings(Collection<PathFragment> paths) {
     ImmutableSet.Builder<String> result = ImmutableSet.builder();
     for (PathFragment path : paths) {
       result.add(path.getSafePathString());
     }
-    return result.build().asList();
+    return result.build();
   }
 
   /**
    * Select .pcm inputs to pass on the command line depending on whether we are in pic or non-pic
    * mode.
    */
-  private ImmutableList<String> getHeaderModulePaths(
+  private ImmutableSet<String> getHeaderModulePaths(
       CppCompileActionBuilder builder, boolean usePic) {
     ImmutableSet.Builder<String> result = ImmutableSet.builder();
     Iterable<Artifact> artifacts =
@@ -364,7 +364,7 @@ public final class CppModel {
     for (Artifact artifact : artifacts) {
       result.add(artifact.getExecPathString());
     }
-    return result.build().asList();
+    return result.build();
   }
 
   private void setupCompileBuildVariables(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcVariablesExtension.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcVariablesExtension.java
index 8d872b0493..f8e3802489 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcVariablesExtension.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcVariablesExtension.java
@@ -170,22 +170,21 @@ class ObjcVariablesExtension implements VariablesExtension {
         FULLY_LINKED_ARCHIVE_PATH_VARIABLE_NAME, fullyLinkArchive.getExecPathString());
     builder.addStringSequenceVariable(
         OBJC_LIBRARY_EXEC_PATHS_VARIABLE_NAME,
-        ImmutableList.copyOf(Artifact.toExecPaths(objcProvider.getObjcLibraries())));
+        Artifact.toExecPaths(objcProvider.getObjcLibraries()));
     builder.addStringSequenceVariable(
         CC_LIBRARY_EXEC_PATHS_VARIABLE_NAME,
-        ImmutableList.copyOf(Artifact.toExecPaths(objcProvider.getCcLibraries())));
+        Artifact.toExecPaths(objcProvider.getCcLibraries()));
     builder.addStringSequenceVariable(
         IMPORTED_LIBRARY_EXEC_PATHS_VARIABLE_NAME,
-        ImmutableList.copyOf(Artifact.toExecPaths(objcProvider.get(IMPORTED_LIBRARY))));
+        Artifact.toExecPaths(objcProvider.get(IMPORTED_LIBRARY)));
   }
 
   private void addExecutableLinkVariables(CcToolchainFeatures.Variables.Builder builder) {
     builder.addStringSequenceVariable(
-        FRAMEWORK_NAMES_VARIABLE_NAME, ImmutableList.copyOf(frameworkNames));
+        FRAMEWORK_NAMES_VARIABLE_NAME, frameworkNames);
     builder.addStringSequenceVariable(
         WEAK_FRAMEWORK_NAMES_VARIABLE_NAME,
-        ImmutableList.copyOf(
-            SdkFramework.names(objcProvider.get(ObjcProvider.WEAK_SDK_FRAMEWORK))));
+            SdkFramework.names(objcProvider.get(ObjcProvider.WEAK_SDK_FRAMEWORK)));
     builder.addStringSequenceVariable(LIBRARY_NAMES_VARIABLE_NAME, libraryNames);
     builder.addStringVariable(
         FILELIST_VARIABLE_NAME, intermediateArtifacts.linkerObjList().getExecPathString());
@@ -197,7 +196,7 @@ class ObjcVariablesExtension implements VariablesExtension {
 
     builder.addStringSequenceVariable(
         FORCE_LOAD_EXEC_PATHS_VARIABLE_NAME,
-        ImmutableList.copyOf(Artifact.toExecPaths(forceLoadArtifacts)));
+        Artifact.toExecPaths(forceLoadArtifacts));
     builder.addStringSequenceVariable(DEP_LINKOPTS_VARIABLE_NAME, objcProvider.get(LINKOPT));
     builder.addStringSequenceVariable(ATTR_LINKOPTS_VARIABLE_NAME, attributeLinkopts);
   }