Skip to content

Commit

Permalink
Optimizes performance of ActionFS staging and eliminates ActionFS upd…
Browse files Browse the repository at this point in the history 
…ates.

Extracts a class, InputArtifactData to hold the input data instead of using a raw map. This provides the flexibility needed to support both ActionFS and existing code so ActionFS does not need to rekey the input data.

Uses the smaller, getDeclaredIncludeSrcs instead of getAllowedDerivedInputs
when possible for staging optional inputs in ActionFS.

PiperOrigin-RevId: 196736703
  • Loading branch information
@aoeui
aoeui authored and Copybara-Service committed May 15, 2018
1 parent 374cae6 commit 0015d18
Show file tree
Hide file tree
Showing 11 changed files with 318 additions and 357 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/graph",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/profiler/memory:current_rule_tracker",
"//src/main/java/com/google/devtools/build/lib/rules/cpp:cpp_interface",
"//src/main/java/com/google/devtools/build/lib/shell",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/skylarkbuildapi",
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ public enum ProfilerTask {
SKYLARK_BUILTIN_FN("Skylark builtin function call", -1, 0x990033, 0),
SKYLARK_USER_COMPILED_FN("Skylark compiled user function call", -1, 0xCC0033, 0),
ACTION_FS_STAGING("Staging per-action file system", -1, 0x000000, 0),
ACTION_FS_UPDATE("Updating per-action file system", -1, 0x000000, 0),
UNKNOWN("Unknown event", -1, 0x339966, 0);

// Size of the ProfilerTask value space.
Expand Down
27 changes: 23 additions & 4 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,23 @@ filegroup(
visibility = ["//src/main/java/com/google/devtools/build/lib:__pkg__"],
)

INTERFACE_SOURCES = [
"IncludeScannable.java",
]

# Description:
# C++ rule support
java_library(
name = "cpp",
srcs = glob([
"*.java",
"transitions/*.java",
]),
srcs = glob(
[
"*.java",
"transitions/*.java",
],
exclude = INTERFACE_SOURCES,
),
deps = [
":cpp_interface",
"//src/main/java/com/google/devtools/build/lib:build-base",
"//src/main/java/com/google/devtools/build/lib:events",
"//src/main/java/com/google/devtools/build/lib:io",
Expand Down Expand Up @@ -45,3 +53,14 @@ java_library(
"//third_party/protobuf:protobuf_java",
],
)

java_library(
name = "cpp_interface",
srcs = INTERFACE_SOURCES,
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//third_party:jsr305",
],
)
83 changes: 46 additions & 37 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

import com.google.common.base.Function;
import com.google.common.base.Preconditions;
import com.google.common.base.Predicates;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand All @@ -42,6 +41,8 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.rules.cpp.IncludeScannable;
import com.google.devtools.build.lib.skyframe.InputArtifactData.MutableInputArtifactData;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -149,8 +150,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws ActionExecutionFu
env.getValues(state.allInputs.keysRequested);
Preconditions.checkState(!env.valuesMissing(), "%s %s", action, state);
}
Pair<Map<Artifact, FileArtifactValue>, Map<Artifact, Collection<Artifact>>> checkedInputs =
null;
Pair<MutableInputArtifactData, Map<Artifact, Collection<Artifact>>> checkedInputs = null;
try {
// Declare deps on known inputs to action. We do this unconditionally to maintain our
// invariant of asking for the same deps each build.
Expand Down Expand Up @@ -190,14 +190,29 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws ActionExecutionFu
state.inputArtifactData = checkedInputs.first;
state.expandedArtifacts = checkedInputs.second;
if (skyframeActionExecutor.usesActionFileSystem()) {
Iterable<Artifact> optionalInputs;
if (action.discoversInputs()) {
if (action instanceof IncludeScannable) {
// This is a performance optimization to minimize nested set traversals for cpp
// compilation. CppCompileAction.getAllowedDerivedInputs iterates over mandatory inputs,
// prunable inputs, declared include srcs, transitive compilation prerequisites and
// transitive modules.
//
// The only one of those sets known to be needed is the declared include srcs.
optionalInputs = ((IncludeScannable) action).getDeclaredIncludeSrcs();
} else {
// This might be reachable by LtoBackendAction and ExtraAction.
optionalInputs = action.getAllowedDerivedInputs();
}
} else {
optionalInputs = ImmutableList.of();
}
state.actionFileSystem =
new ActionFileSystem(
skyframeActionExecutor.getExecRoot(),
skyframeActionExecutor.getSourceRoots(),
checkedInputs.first,
// TODO(shahan): based on experimentation, it suffices to include only {@link
// com.google.devtools.build.lib.rules.cpp.IncludeScannable#getDeclaredIncludeSrcs}.
// That may be a smaller set and more efficient but makes this class have an
// awkward dependency on something in the cpp package.
action.discoversInputs() ? action.getAllowedDerivedInputs() : ImmutableList.of(),
optionalInputs,
action.getOutputs());
}
}
Expand Down Expand Up @@ -428,7 +443,6 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
}
perActionFileCache =
new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false);
state.updateFileSystemInputData();

// Stage 1 finished, let's do stage 2. The stage 1 of input discovery will have added some
// files with addDiscoveredInputs() and then have waited for those files to be available
Expand All @@ -446,7 +460,6 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
}
perActionFileCache =
new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false);
state.updateFileSystemInputData();
}
metadataHandler =
new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get());
Expand Down Expand Up @@ -502,8 +515,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
}

if (action.discoversInputs()) {
Iterable<Artifact> newInputs =
filterKnownInputs(action.getInputs(), state.inputArtifactData.keySet());
Iterable<Artifact> newInputs = filterKnownInputs(action.getInputs(), state.inputArtifactData);
Map<SkyKey, SkyValue> metadataFoundDuringActionExecution =
env.getValues(toKeys(newInputs, action.getMandatoryInputs()));
state.discoveredInputs = newInputs;
Expand All @@ -514,13 +526,10 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
// We are in the interesting case of an action that discovered its inputs during
// execution, and found some new ones, but the new ones were already present in the graph.
// We must therefore cache the metadata for those new ones.
Map<Artifact, FileArtifactValue> inputArtifactData = new HashMap<>();
inputArtifactData.putAll(state.inputArtifactData);
for (Map.Entry<SkyKey, SkyValue> entry : metadataFoundDuringActionExecution.entrySet()) {
inputArtifactData.put(
state.inputArtifactData.put(
ArtifactSkyKey.artifact(entry.getKey()), (FileArtifactValue) entry.getValue());
}
state.inputArtifactData = inputArtifactData;
// TODO(ulfjack): This causes information loss about omitted and injected outputs. Also see
// the documentation on MetadataHandler.artifactOmitted. This works by accident because
// markOmitted is only called for remote execution, and this code only gets executed for
Expand All @@ -545,13 +554,13 @@ public SkyKey apply(@Nullable Artifact artifact) {
};

private static Iterable<SkyKey> newlyDiscoveredInputsToSkyKeys(
Iterable<Artifact> discoveredInputs, Set<Artifact> knownInputs) {
Iterable<Artifact> discoveredInputs, MutableInputArtifactData inputArtifactData) {
return Iterables.transform(
filterKnownInputs(discoveredInputs, knownInputs), TO_NONMANDATORY_SKYKEY);
filterKnownInputs(discoveredInputs, inputArtifactData), TO_NONMANDATORY_SKYKEY);
}

private static void addDiscoveredInputs(
Map<Artifact, FileArtifactValue> inputData,
MutableInputArtifactData inputData,
Map<Artifact, Collection<Artifact>> expandedArtifacts,
Iterable<Artifact> discoveredInputs,
Environment env)
Expand All @@ -564,15 +573,17 @@ private static void addDiscoveredInputs(
// discovery.
// Therefore there is no need to catch and rethrow exceptions as there is with #checkInputs.
Map<SkyKey, SkyValue> nonMandatoryDiscovered =
env.getValues(newlyDiscoveredInputsToSkyKeys(discoveredInputs, inputData.keySet()));
env.getValues(newlyDiscoveredInputsToSkyKeys(discoveredInputs, inputData));
if (!env.valuesMissing()) {
for (Map.Entry<SkyKey, SkyValue> entry : nonMandatoryDiscovered.entrySet()) {
Artifact input = ArtifactSkyKey.artifact(entry.getKey());
if (entry.getValue() instanceof TreeArtifactValue) {
TreeArtifactValue treeValue = (TreeArtifactValue) entry.getValue();
expandedArtifacts.put(input, ImmutableSet.<Artifact>copyOf(treeValue.getChildren()));
inputData.putAll(treeValue.getChildValues());

for (Map.Entry<Artifact.TreeFileArtifact, FileArtifactValue> child :
treeValue.getChildValues().entrySet()) {
inputData.put(child.getKey(), child.getValue());
}
inputData.put(input, treeValue.getSelfData());
} else {
inputData.put(input, (FileArtifactValue) entry.getValue());
Expand Down Expand Up @@ -632,8 +643,9 @@ public SkyKey apply(Artifact artifact) {
* Declare dependency on all known inputs of action. Throws exception if any are known to be
* missing. Some inputs may not yet be in the graph, in which case the builder should abort.
*/
private Pair<Map<Artifact, FileArtifactValue>, Map<Artifact, Collection<Artifact>>>
checkInputs(Environment env, Action action,
private Pair<MutableInputArtifactData, Map<Artifact, Collection<Artifact>>> checkInputs(
Environment env,
Action action,
Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps)
throws ActionExecutionException {
int missingCount = 0;
Expand All @@ -644,8 +656,8 @@ public SkyKey apply(Artifact artifact) {
// some deps are still missing.
boolean populateInputData = !env.valuesMissing();
NestedSetBuilder<Cause> rootCauses = NestedSetBuilder.stableOrder();
Map<Artifact, FileArtifactValue> inputArtifactData =
new HashMap<>(populateInputData ? inputDeps.size() : 0);
MutableInputArtifactData inputArtifactData =
new MutableInputArtifactData(populateInputData ? inputDeps.size() : 0);
Map<Artifact, Collection<Artifact>> expandedArtifacts =
new HashMap<>(populateInputData ? 128 : 0);

Expand All @@ -672,7 +684,10 @@ public SkyKey apply(Artifact artifact) {
} else if (value instanceof TreeArtifactValue) {
TreeArtifactValue treeValue = (TreeArtifactValue) value;
expandedArtifacts.put(input, ImmutableSet.<Artifact>copyOf(treeValue.getChildren()));
inputArtifactData.putAll(treeValue.getChildValues());
for (Map.Entry<Artifact.TreeFileArtifact, FileArtifactValue> child :
treeValue.getChildValues().entrySet()) {
inputArtifactData.put(child.getKey(), child.getValue());
}

// Again, we cache the "digest" of the value for cache checking.
inputArtifactData.put(input, treeValue.getSelfData());
Expand Down Expand Up @@ -729,8 +744,8 @@ public SkyKey apply(Artifact artifact) {
}

private static Iterable<Artifact> filterKnownInputs(
Iterable<Artifact> newInputs, Set<Artifact> knownInputs) {
return Iterables.filter(newInputs, Predicates.not(Predicates.in(knownInputs)));
Iterable<Artifact> newInputs, MutableInputArtifactData inputArtifactData) {
return Iterables.filter(newInputs, input -> !inputArtifactData.contains(input));
}

/**
Expand Down Expand Up @@ -785,7 +800,8 @@ private ContinuationState getState(Action action) {
private static class ContinuationState {
AllInputs allInputs;
/** Mutable map containing metadata for known artifacts. */
Map<Artifact, FileArtifactValue> inputArtifactData = null;
MutableInputArtifactData inputArtifactData = null;

Map<Artifact, Collection<Artifact>> expandedArtifacts = null;
Token token = null;
Iterable<Artifact> discoveredInputs = null;
Expand Down Expand Up @@ -821,13 +837,6 @@ void updateFileSystemContext(
}
}

/** Propagates {@link inputArtifactData} changes due to input discovery. */
void updateFileSystemInputData() {
if (actionFileSystem != null) {
actionFileSystem.updateInputData(inputArtifactData);
}
}

@Override
public String toString() {
return token + ", " + value + ", " + allInputs + ", " + inputArtifactData + ", "
Expand Down
Loading

0 comments on commit 0015d18

Please sign in to comment.