Skip to content

Commit

Permalink
Add the path for in-memory jdeps files to execution info on demand in…
Browse files Browse the repository at this point in the history
…stead of storing it in `JavaCompileAction` and `JavaHeaderCompileAction`.

Fixes memory overhead caused by `--experimental_inmemory_jdeps_files`.

The jdeps path added to the execution info map is novel for a given action, resulting in interning via `SpawnAction#executionInfoInterner` backfiring for `JavaHeaderCompileAction` - we get the interner overhead without the benefit, since entries are never equal. Instead, add the extra map entry when `getExecutionInfo()` is called. This approach is similar to how we implement `--experimental_inmemory_dotd_files`, see `CppCompileAction`.

This change means that `--modify_execution_info` can't strip the `internal-inline-outputs` key, which is actually a good thing given that it's internal, and is consistent with in-memory .d files.

PiperOrigin-RevId: 615419908
Change-Id: I48a5382007a5a3ebe246817a4723c9cef6314076
  • Loading branch information
justinhorvitz authored and fmeum committed Mar 14, 2024
1 parent 3e8ee27 commit c830f21
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import com.google.devtools.build.lib.actions.EmptyRunfilesSupplier;
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.ParamFileInfo;
import com.google.devtools.build.lib.actions.ParameterFile;
import com.google.devtools.build.lib.actions.PathMapper;
Expand Down Expand Up @@ -661,7 +662,16 @@ public Sequence<String> getStarlarkArgv() throws EvalException, InterruptedExcep
/** Returns the out-of-band execution data for this action. */
@Override
public ImmutableMap<String, String> getExecutionInfo() {
return mergeMaps(super.getExecutionInfo(), executionInfo);
var result = mergeMaps(super.getExecutionInfo(), executionInfo);
if (outputDepsProto == null
|| !configuration.getFragment(JavaConfiguration.class).inmemoryJdepsFiles()) {
return result;
}
return mergeMaps(
result,
ImmutableMap.of(
ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS,
outputDepsProto.getExecPathString()));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
import com.google.devtools.build.lib.actions.extra.JavaCompileInfo;
import com.google.devtools.build.lib.analysis.RuleContext;
Expand Down Expand Up @@ -230,20 +229,6 @@ public JavaCompileAction build() throws RuleErrorException {
classpathMode = JavaClasspathMode.OFF;
}

if (outputs.depsProto() != null) {
JavaConfiguration javaConfiguration =
ruleContext.getConfiguration().getFragment(JavaConfiguration.class);
if (javaConfiguration.inmemoryJdepsFiles()) {
executionInfo =
ImmutableMap.<String, String>builderWithExpectedSize(this.executionInfo.size() + 1)
.putAll(this.executionInfo)
.put(
ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS,
outputs.depsProto().getExecPathString())
.buildOrThrow();
}
}

NestedSet<Artifact> tools = toolsBuilder.build();
mandatoryInputsBuilder.addTransitive(tools);
NestedSet<Artifact> mandatoryInputs = mandatoryInputsBuilder.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.devtools.build.lib.actions.ActionAnalysisMetadata.mergeMaps;
import static com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType.UNQUOTED;
import static com.google.devtools.build.lib.rules.java.JavaCompileActionBuilder.UTF8_ENVIRONMENT;
import static java.nio.charset.StandardCharsets.ISO_8859_1;
Expand Down Expand Up @@ -80,6 +81,7 @@ public final class JavaHeaderCompileAction extends SpawnAction {
private static final String DIRECT_CLASSPATH_MNEMONIC = "Turbine";

private final boolean insertDependencies;
private final boolean inMemoryJdeps;
private final NestedSet<Artifact> additionalArtifactsForPathMapping;

private JavaHeaderCompileAction(
Expand All @@ -96,6 +98,7 @@ private JavaHeaderCompileAction(
String mnemonic,
OutputPathsMode outputPathsMode,
boolean insertDependencies,
boolean inMemoryJdeps,
NestedSet<Artifact> additionalArtifactsForPathMapping) {
super(
owner,
Expand All @@ -111,9 +114,24 @@ private JavaHeaderCompileAction(
mnemonic,
outputPathsMode);
this.insertDependencies = insertDependencies;
this.inMemoryJdeps = inMemoryJdeps;
this.additionalArtifactsForPathMapping = additionalArtifactsForPathMapping;
}

@Override
public ImmutableMap<String, String> getExecutionInfo() {
var result = super.getExecutionInfo();
if (!inMemoryJdeps) {
return result;
}
Artifact outputDepsProto = Iterables.get(getOutputs(), 1);
return mergeMaps(
result,
ImmutableMap.of(
ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS,
outputDepsProto.getExecPathString()));
}

@Override
public NestedSet<Artifact> getAdditionalArtifactsForPathMapping() {
return additionalArtifactsForPathMapping;
Expand Down Expand Up @@ -478,11 +496,6 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
executionInfo.putAll(
TargetUtils.getExecutionInfo(
ruleContext.getRule(), ruleContext.isAllowTagsPropagation()));
if (javaConfiguration.inmemoryJdepsFiles()) {
executionInfo.put(
ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS,
outputDepsProto.getExecPathString());
}

if (useDirectClasspath) {
NestedSet<Artifact> classpath;
Expand Down Expand Up @@ -535,6 +548,7 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
// available to downstream actions. Else just do enough work to locally create the
// full .jdeps from the .stripped .jdeps produced on the executor.
/* insertDependencies= */ classpathMode == JavaClasspathMode.BAZEL,
javaConfiguration.inmemoryJdepsFiles(),
additionalArtifactsForPathMapping));
return;
}
Expand Down

0 comments on commit c830f21

Please sign in to comment.