From 0ebad395a2d4321ea0c4130faacb9f4810e615c7 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 1 Apr 2022 16:24:36 +0200 Subject: [PATCH] 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. --- .../build/lib/actions/ActionEnvironment.java | 24 +++++++++++-------- .../lib/analysis/test/TestActionBuilder.java | 2 +- .../rules/java/JavaCompileActionBuilder.java | 3 ++- .../java/JavaHeaderCompileActionBuilder.java | 3 ++- .../lib/actions/ActionEnvironmentTest.java | 17 +++++++++++-- 5 files changed, 34 insertions(+), 15 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 index f7bad5cf5529d0..ecd1d3d1a93005 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java @@ -81,10 +81,10 @@ default int size() { */ static class CompoundEnvironmentVariables implements EnvironmentVariables { - static EnvironmentVariables create( - Map fixedVars, Set inheritedVars, EnvironmentVariables base) { - if (fixedVars.isEmpty() && inheritedVars.isEmpty() && base.isEmpty()) { - return EMPTY_ENVIRONMENT_VARIABLES; + static EnvironmentVariables create(Map fixedVars, Set inheritedVars, + EnvironmentVariables base) { + if (fixedVars.isEmpty() && inheritedVars.isEmpty()) { + return base; } return new CompoundEnvironmentVariables(fixedVars, inheritedVars, base); } @@ -218,18 +218,22 @@ public static ActionEnvironment create(Map fixedEnv) { * Returns a copy of the environment with the given fixed variables added to it, overwriting * any existing occurrences of those variables. */ - public ActionEnvironment addFixedVariables(Map fixedVars) { - return ActionEnvironment.create( - CompoundEnvironmentVariables.create(fixedVars, ImmutableSet.of(), vars)); + public ActionEnvironment withAdditionalFixedVariables(Map fixedVars) { + return withAdditionalVariables(fixedVars, ImmutableSet.of()); } /** * Returns a copy of the environment with the given fixed and inherited variables added to it, * overwriting any existing occurrences of those variables. */ - public ActionEnvironment addVariables(Map fixedVars, Set inheritedVars) { - return ActionEnvironment.create( - CompoundEnvironmentVariables.create(fixedVars, inheritedVars, vars)); + public ActionEnvironment withAdditionalVariables(Map fixedVars, + Set 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 index 5825c2ab70b3fa..d7cb43efcd787b 100644 --- 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 index 214cc1060c7b8e..b6f619667ae0a2 100644 --- 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,8 @@ public JavaCompileAction build() { toolsBuilder.addTransitive(toolsJars); ActionEnvironment actionEnvironment = - ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT); + ruleContext.getConfiguration().getActionEnvironment() + .withAdditionalFixedVariables(UTF8_ENVIRONMENT); NestedSetBuilder 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 index 0838f8667e1c75..1ea4fb4ef83f0f 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 @@ -296,7 +296,8 @@ 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 index 5a49df579f8c37..83a2bc4ea72729 100644 --- 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(ImmutableMap.of("FIXED_AND_INHERITED", "fixed"), + .withAdditionalVariables(ImmutableMap.of("FIXED_AND_INHERITED", "fixed"), ImmutableSet.of("FIXED_AND_INHERITED")); Map clientEnv = ImmutableMap.of("INHERITED_ONLY", "inherited", "FIXED_AND_INHERITED", "inherited"); @@ -58,4 +58,17 @@ public void fixedInheritedInteraction() { assertThat(result).containsExactly("FIXED_ONLY", "fixed", "FIXED_AND_INHERITED", "inherited", "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); + } }