Skip to content

Commit

Permalink
Intern trivial ActionEnvironment and EnvironmentVariables instances
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
fmeum committed Apr 4, 2022
1 parent 3fff4b9 commit 0ebad39
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,10 @@ default int size() {
*/
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;
static EnvironmentVariables create(Map<String, String> fixedVars, Set<String> inheritedVars,
EnvironmentVariables base) {
if (fixedVars.isEmpty() && inheritedVars.isEmpty()) {
return base;
}
return new CompoundEnvironmentVariables(fixedVars, inheritedVars, base);
}
Expand Down Expand Up @@ -218,18 +218,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);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ private TestParams createTestAction(int shards)
testProperties,
runfilesSupport
.getActionEnvironment()
.addVariables(extraTestEnv, extraInheritedEnv),
.withAdditionalVariables(extraTestEnv, extraInheritedEnv),
executionSettings,
shard,
run,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Artifact> mandatoryInputsBuilder = NestedSetBuilder.stableOrder();
mandatoryInputsBuilder
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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<String, String> clientEnv = ImmutableMap.of("INHERITED_ONLY", "inherited",
"FIXED_AND_INHERITED", "inherited");
Expand All @@ -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);
}
}

0 comments on commit 0ebad39

Please sign in to comment.