Intern trivial ActionEnvironment and EnvironmentVariables instances

When an ActionEnvironment is constructed out of an existing one, the ActionEnvironment and EnvironmentVariables instances, which are immutable, can be reused if no variables are added. Also renames addVariables and addFixedVariables to better reflect that they are operating on an immutable type. Closes #15171. PiperOrigin-RevId: 440312159
bazelbuild · Apr 8, 2022 · 01d337f · 01d337f
1 parent 3f2e8c8
commit 01d337f
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 13 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java b/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java
@@ -83,8 +83,8 @@ static class CompoundEnvironmentVariables implements EnvironmentVariables {
 
     static EnvironmentVariables create(
         Map<String, String> fixedVars, Set<String> inheritedVars, EnvironmentVariables base) {
-      if (fixedVars.isEmpty() && inheritedVars.isEmpty() && base.isEmpty()) {
-        return EMPTY_ENVIRONMENT_VARIABLES;
+      if (fixedVars.isEmpty() && inheritedVars.isEmpty()) {
+        return base;
       }
       return new CompoundEnvironmentVariables(fixedVars, inheritedVars, base);
     }
@@ -219,18 +219,22 @@ public static ActionEnvironment create(Map<String, String> fixedEnv) {
    * Returns a copy of the environment with the given fixed variables added to it, <em>overwriting
    * any existing occurrences of those variables</em>.
    */
-  public ActionEnvironment addFixedVariables(Map<String, String> fixedVars) {
-    return ActionEnvironment.create(
-        CompoundEnvironmentVariables.create(fixedVars, ImmutableSet.of(), vars));
+  public ActionEnvironment withAdditionalFixedVariables(Map<String, String> fixedVars) {
+    return withAdditionalVariables(fixedVars, ImmutableSet.of());
   }
 
   /**
    * Returns a copy of the environment with the given fixed and inherited variables added to it,
    * <em>overwriting any existing occurrences of those variables</em>.
    */
-  public ActionEnvironment addVariables(Map<String, String> fixedVars, Set<String> inheritedVars) {
-    return ActionEnvironment.create(
-        CompoundEnvironmentVariables.create(fixedVars, inheritedVars, vars));
+  public ActionEnvironment withAdditionalVariables(
+      Map<String, String> fixedVars, Set<String> inheritedVars) {
+    EnvironmentVariables newVars =
+        CompoundEnvironmentVariables.create(fixedVars, inheritedVars, vars);
+    if (newVars == vars) {
+      return this;
+    }
+    return ActionEnvironment.create(newVars);
   }
 
   /**

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
@@ -413,7 +413,7 @@ private TestParams createTestAction(int shards)
                 testProperties,
                 runfilesSupport
                     .getActionEnvironment()
-                    .addVariables(extraTestEnv, extraInheritedEnv),
+                    .withAdditionalVariables(extraTestEnv, extraInheritedEnv),
                 executionSettings,
                 shard,
                 run,

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java
@@ -205,7 +205,10 @@ public JavaCompileAction build() {
     toolsBuilder.addTransitive(toolsJars);
 
     ActionEnvironment actionEnvironment =
-        ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT);
+        ruleContext
+            .getConfiguration()
+            .getActionEnvironment()
+            .withAdditionalFixedVariables(UTF8_ENVIRONMENT);
 
     NestedSetBuilder<Artifact> mandatoryInputsBuilder = NestedSetBuilder.stableOrder();
     mandatoryInputsBuilder

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
@@ -296,7 +296,10 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti
     }
 
     ActionEnvironment actionEnvironment =
-        ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT);
+        ruleContext
+            .getConfiguration()
+            .getActionEnvironment()
+            .withAdditionalFixedVariables(UTF8_ENVIRONMENT);
 
     OnDemandString progressMessage =
         new ProgressMessage(

diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java
@@ -34,7 +34,7 @@ public void compoundEnvOrdering() {
         ActionEnvironment.create(
             ImmutableMap.of("FOO", "foo1", "BAR", "bar"), ImmutableSet.of("baz"));
     // entries added by env2 override the existing entries
-    ActionEnvironment env2 = env1.addFixedVariables(ImmutableMap.of("FOO", "foo2"));
+    ActionEnvironment env2 = env1.withAdditionalFixedVariables(ImmutableMap.of("FOO", "foo2"));
 
     assertThat(env1.getFixedEnv()).containsExactly("FOO", "foo1", "BAR", "bar");
     assertThat(env1.getInheritedEnv()).containsExactly("baz");
@@ -48,7 +48,7 @@ public void fixedInheritedInteraction() {
     ActionEnvironment env =
         ActionEnvironment.create(
                 ImmutableMap.of("FIXED_ONLY", "fixed"), ImmutableSet.of("INHERITED_ONLY"))
-            .addVariables(
+            .withAdditionalVariables(
                 ImmutableMap.of("FIXED_AND_INHERITED", "fixed"),
                 ImmutableSet.of("FIXED_AND_INHERITED"));
     Map<String, String> clientEnv =
@@ -65,4 +65,17 @@ public void fixedInheritedInteraction() {
             "INHERITED_ONLY",
             "inherited");
   }
+
+  @Test
+  public void emptyEnvironmentInterning() {
+    ActionEnvironment emptyEnvironment =
+        ActionEnvironment.create(ImmutableMap.of(), ImmutableSet.of());
+    assertThat(emptyEnvironment).isSameInstanceAs(ActionEnvironment.EMPTY);
+
+    ActionEnvironment base =
+        ActionEnvironment.create(ImmutableMap.of("FOO", "foo1"), ImmutableSet.of("baz"));
+    assertThat(base.withAdditionalFixedVariables(ImmutableMap.of())).isSameInstanceAs(base);
+    assertThat(base.withAdditionalVariables(ImmutableMap.of(), ImmutableSet.of()))
+        .isSameInstanceAs(base);
+  }
 }