Skip to content

Commit

Permalink
Remove ActionMetadataHandler.transformAfterInputDiscovery().
Browse files Browse the repository at this point in the history 
It doesn't change anything in ActionMetadataHandler now that getInputMetadata()
does not throw on unknown inputs anymore.

RELNOTES: None.
PiperOrigin-RevId: 523978022
Change-Id: Id4837071a778c26f39cda8c703d8c0a1a3d01ad4
  • Loading branch information
@lberki @copybara-github
lberki authored and copybara-github committed Apr 13, 2023
1 parent 73d937d commit 42ae8a5
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 99 deletions.
39 changes: 10 additions & 29 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 @@ -748,7 +748,6 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
tsgm.get(),
pathResolver,
skyframeActionExecutor.getExecRoot().asFragment(),
PathFragment.create(directories.getRelativeOutputPath()),
expandedFilesets);

// We only need to check the action cache if we haven't done it on a previous run.
Expand Down Expand Up @@ -808,15 +807,12 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
"Input discovery returned null but no more deps were requested by %s",
action);
}
switch (addDiscoveredInputs(state, env, action)) {
case VALUES_MISSING:
return null;
case NO_DISCOVERED_DATA:
break;
case DISCOVERED_DATA:
metadataHandler = metadataHandler.transformAfterInputDiscovery(new OutputStore());
metadataHandler.prepareForActionExecution();

addDiscoveredInputs(state, env, action);
if (env.valuesMissing()) {
return null;
}

// When discover inputs completes, post an event with the duration values.
env.getListener()
.post(
Expand Down Expand Up @@ -866,17 +862,9 @@ public void run(
state.getExpandedFilesets();
if (action.discoversInputs()) {
state.discoveredInputs = action.getInputs();
switch (addDiscoveredInputs(state, env, action)) {
case VALUES_MISSING:
return;
case NO_DISCOVERED_DATA:
break;
case DISCOVERED_DATA:
// 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.
metadataHandler =
metadataHandler.transformAfterInputDiscovery(metadataHandler.getOutputStore());
addDiscoveredInputs(state, env, action);
if (env.valuesMissing()) {
return;
}
}
checkState(!env.valuesMissing(), action);
Expand All @@ -895,13 +883,7 @@ public void run(
}
}

private enum DiscoveredState {
VALUES_MISSING,
NO_DISCOVERED_DATA,
DISCOVERED_DATA
}

private DiscoveredState addDiscoveredInputs(
private void addDiscoveredInputs(
InputDiscoveryState state, Environment env, Action actionForError)
throws InterruptedException, ActionExecutionException {
// TODO(janakr): This code's assumptions are wrong in the face of Starlark actions with unused
Expand All @@ -919,7 +901,7 @@ private DiscoveredState addDiscoveredInputs(
}

if (unknownDiscoveredInputs.isEmpty()) {
return DiscoveredState.NO_DISCOVERED_DATA;
return;
}

SkyframeLookupResult nonMandatoryDiscovered =
Expand Down Expand Up @@ -980,7 +962,6 @@ private DiscoveredState addDiscoveredInputs(
"unknown metadata for " + input.getExecPathString() + ": " + retrievedMetadata);
}
}
return env.valuesMissing() ? DiscoveredState.VALUES_MISSING : DiscoveredState.DISCOVERED_DATA;
}

@Nullable
Expand Down
36 changes: 0 additions & 36 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,6 @@ final class ActionMetadataHandler implements InputMetadataProvider, OutputMetada

/**
* Creates a new metadata handler.
*
* <p>If the handler is for use during input discovery, calling {@link #getMetadata} with an
* artifact which is neither in {@code inputArtifactData} nor {@code outputs} is tolerated and
* will return {@code null}. To subsequently transform the handler for regular action execution
* (where such a call is not permitted), use {@link #transformAfterInputDiscovery}.
*/
static ActionMetadataHandler create(
ActionInputMap inputArtifactData,
Expand All @@ -100,7 +95,6 @@ static ActionMetadataHandler create(
TimestampGranularityMonitor tsgm,
ArtifactPathResolver artifactPathResolver,
PathFragment execRoot,
PathFragment derivedPathPrefix,
Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets) {
return new ActionMetadataHandler(
inputArtifactData,
Expand All @@ -111,7 +105,6 @@ static ActionMetadataHandler create(
tsgm,
artifactPathResolver,
execRoot,
derivedPathPrefix,
createFilesetMapping(expandedFilesets, execRoot),
new OutputStore());
}
Expand All @@ -128,7 +121,6 @@ static ActionMetadataHandler create(
private final TimestampGranularityMonitor tsgm;
private final ArtifactPathResolver artifactPathResolver;
private final PathFragment execRoot;
private final PathFragment derivedPathPrefix;

private final AtomicBoolean executionMode = new AtomicBoolean(false);
private final OutputStore store;
Expand All @@ -142,7 +134,6 @@ private ActionMetadataHandler(
TimestampGranularityMonitor tsgm,
ArtifactPathResolver artifactPathResolver,
PathFragment execRoot,
PathFragment derivedPathPrefix,
ImmutableMap<PathFragment, FileArtifactValue> filesetMapping,
OutputStore store) {
this.inputArtifactData = checkNotNull(inputArtifactData);
Expand All @@ -153,37 +144,10 @@ private ActionMetadataHandler(
this.tsgm = checkNotNull(tsgm);
this.artifactPathResolver = checkNotNull(artifactPathResolver);
this.execRoot = checkNotNull(execRoot);
this.derivedPathPrefix = checkNotNull(derivedPathPrefix);
this.filesetMapping = checkNotNull(filesetMapping);
this.store = checkNotNull(store);
}

/**
* Returns a new handler mostly identical to this one, except uses the given {@code store} and
* does not permit {@link #getMetadata} to be called with an artifact which is neither in inputs
* nor outputs.
*
* <p>The returned handler will be in the mode for action cache checking. To prepare it for action
* execution, call {@link #prepareForActionExecution}.
*
* <p>This method is designed to be called after input discovery when a fresh handler is needed
* but all of the parameters in {@link #create} would be the same as the original handler.
*/
ActionMetadataHandler transformAfterInputDiscovery(OutputStore store) {
return new ActionMetadataHandler(
inputArtifactData,
archivedTreeArtifactsEnabled,
outputPermissions,
outputs,
xattrProvider,
tsgm,
artifactPathResolver,
execRoot,
derivedPathPrefix,
filesetMapping,
store);
}

/**
* If {@code value} represents an existing file, returns it as is, otherwise throws {@link
* FileNotFoundException}.
Expand Down
34 changes: 0 additions & 34 deletions src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ private ActionMetadataHandler createHandler(
tsgm,
ArtifactPathResolver.IDENTITY,
execRoot.asFragment(),
derivedPathPrefix,
/* expandedFilesets= */ ImmutableMap.of());
}

Expand Down Expand Up @@ -420,7 +419,6 @@ public void getMetadataFromFilesetMapping() throws Exception {
tsgm,
ArtifactPathResolver.IDENTITY,
execRoot.asFragment(),
derivedPathPrefix,
expandedFilesets);

// Only the regular FileArtifactValue should have its metadata stored.
Expand Down Expand Up @@ -530,7 +528,6 @@ public void outputArtifactNotPreviouslyInjectedInExecutionMode_writablePermissio
tsgm,
ArtifactPathResolver.IDENTITY,
execRoot.asFragment(),
derivedPathPrefix,
/* expandedFilesets= */ ImmutableMap.of());
handler.prepareForActionExecution();

Expand Down Expand Up @@ -581,37 +578,6 @@ public void outputTreeArtifactNotPreviouslyInjectedInExecutionMode() throws Exce
0555);
}

@Test
public void transformAfterInputDiscovery() throws Exception {
Artifact known =
ActionsTestUtil.createArtifactWithRootRelativePath(
outputRoot, PathFragment.create("known"));
Artifact unknown =
ActionsTestUtil.createArtifactWithRootRelativePath(
outputRoot, PathFragment.create("unknown"));
ActionMetadataHandler handler =
createHandler(
new ActionInputMap(0), /* forInputDiscovery= *//* outputs= */ ImmutableSet.of(known));

// Unknown artifact returns null during input discovery.
assertThat(handler.getInputMetadata(unknown)).isNull();

OutputStore newStore = new OutputStore();
FileArtifactValue knownMetadata =
RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1);
newStore.putArtifactData(known, knownMetadata);
ActionMetadataHandler newHandler = handler.transformAfterInputDiscovery(newStore);
assertThat(newHandler.getOutputStore()).isNotEqualTo(handler.getOutputStore());
assertThat(newHandler.getOutputStore()).isEqualTo(newStore);

assertThat(newHandler.getOutputMetadata(known)).isEqualTo(knownMetadata);
// Unknown artifact throws outside of input discovery.
// assertThrows(IllegalStateException.class, () -> newHandler.getInputMetadata(unknown));
// We can transform it again.
assertThat(newHandler.transformAfterInputDiscovery(new OutputStore())).isNotNull();
assertThat(chmodCalls).isEmpty();
}

@Test
public void getTreeArtifactChildren_noData_returnsEmptySet() {
SpecialArtifact treeArtifact =
Expand Down

0 comments on commit 42ae8a5

Please sign in to comment.