Skip to content

Commit

Permalink
Enrich the Skyframe exception handling API with methods that promote …
Browse files Browse the repository at this point in the history 
…code reuse and request batching.

While refactoring `AspectFunction`, I've noticed that Skyframe requests may be suboptimally batched when different keys throw different exception types because the author probably felt that batching them introduced too much complexity.

Another pattern I'm seeing is the use of helper functions which may expect only a certain exception type and/or arity of `ValueOrExceptionN`, leading to suboptimal batching to fit the method's requirements. An example is `ConfiguredTargetFunction#computeDependencies`, where aspects are not requested along with configured targets - requesting them together would make calling into the helper function `ConfiguredTargetFunction#resolveConfiguredTargetDependencies` difficult.

The new methods in this change already have some use cases and will additionally allow me to batch configured target and aspect requests as mentioned above without introducing code duplication.

PiperOrigin-RevId: 412120270
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Nov 24, 2021
1 parent 3069ac4 commit 0a93c1f
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 113 deletions.
39 changes: 13 additions & 26 deletions src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.ValueOrException2;
import com.google.devtools.build.skyframe.ValueOrUntypedException;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -195,12 +196,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

// Keep this in sync with the same code in ConfiguredTargetFunction.
PackageValue packageValue;
try {
packageValue = (PackageValue) baseValues.get(packageKey).get();
} catch (ConfiguredValueCreationException | BzlLoadFailedException e) {
throw new IllegalStateException(e);
}
PackageValue packageValue = (PackageValue) baseValues.get(packageKey).getUnchecked();
if (packageValue == null) {
return null;
}
Expand All @@ -216,12 +212,13 @@ public SkyValue compute(SkyKey skyKey, Environment env)
ConfiguredTargetValue baseConfiguredTargetValue;
try {
baseConfiguredTargetValue =
(ConfiguredTargetValue) baseValues.get(baseConfiguredTargetKey).get();
(ConfiguredTargetValue)
baseValues
.get(baseConfiguredTargetKey)
.getOrThrow(ConfiguredValueCreationException.class);
} catch (ConfiguredValueCreationException e) {
throw new AspectFunctionException(
new AspectCreationException(e.getMessage(), e.getRootCauses(), e.getDetailedExitCode()));
} catch (BzlLoadFailedException e) {
throw new IllegalStateException(e);
}
ConfiguredTarget associatedTarget = baseConfiguredTargetValue.getConfiguredTarget();
Preconditions.checkState(
Expand All @@ -230,16 +227,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
key,
associatedTarget);

BuildConfigurationValue configuration;
if (configurationKey == null) {
configuration = null;
} else {
try {
configuration = (BuildConfigurationValue) baseValues.get(configurationKey).get();
} catch (ConfiguredValueCreationException | BzlLoadFailedException e) {
throw new IllegalStateException(e);
}
}
BuildConfigurationValue configuration =
configurationKey == null
? null
: (BuildConfigurationValue) baseValues.get(configurationKey).getUnchecked();

PackageValue val =
(PackageValue)
Expand Down Expand Up @@ -493,23 +484,19 @@ static StarlarkDefinedAspect loadAspectFromBzl(

@Nullable
private static StarlarkDefinedAspect loadStarlarkAspect(
StarlarkAspectClass starlarkAspectClass,
ValueOrException2<BzlLoadFailedException, ?> bzlLoadValueOrException)
StarlarkAspectClass starlarkAspectClass, ValueOrUntypedException bzlLoadValueOrException)
throws AspectCreationException {
BzlLoadValue bzlLoadValue;
try {
bzlLoadValue = (BzlLoadValue) bzlLoadValueOrException.get();
bzlLoadValue =
(BzlLoadValue) bzlLoadValueOrException.getOrThrow(BzlLoadFailedException.class);
} catch (BzlLoadFailedException e) {
throw new AspectCreationException(
e.getMessage(), starlarkAspectClass.getExtensionLabel(), e.getDetailedExitCode());
} catch (Exception e) {
throw new IllegalStateException(e);
}

if (bzlLoadValue == null) {
return null;
}

return loadAspectFromBzl(starlarkAspectClass, bzlLoadValue);
}

Expand Down
129 changes: 65 additions & 64 deletions src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.ValueOrException;
import com.google.devtools.build.skyframe.ValueOrUntypedException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
Expand Down Expand Up @@ -848,67 +849,9 @@ private static Map<SkyKey, ConfiguredTargetAndData> resolveConfiguredTargetDepen
for (int i = 0; i < 2; i++) {
for (Dependency dep : depsToProcess) {
SkyKey key = dep.getConfiguredTargetKey();
ConfiguredTargetValue depValue;
try {
ConfiguredTargetValue depValue =
(ConfiguredTargetValue) depValuesOrExceptions.get(key).get();

if (depValue == null) {
missedValues = true;
} else {
ConfiguredTarget depCt = depValue.getConfiguredTarget();
Label depLabel = depCt.getLabel();
SkyKey packageKey = PackageValue.key(depLabel.getPackageIdentifier());
PackageValue pkgValue;
if (i == 0) {
ValueOrException<ConfiguredValueCreationException> packageResult =
depValuesOrExceptions.get(packageKey);
if (packageResult == null) {
aliasPackagesToFetch.add(packageKey);
aliasDepsToRedo.add(dep);
continue;
} else {
pkgValue = (PackageValue) packageResult.get();
if (pkgValue == null) {
// In a race, the getValuesOrThrow call above may have retrieved the package
// before it was done but the configured target after it was done. Since
// SkyFunctionEnvironment may cache absent values, re-requesting it on this
// evaluation may be useless, just treat it as missing.
missedValues = true;
continue;
}
}
} else {
// We were doing AliasConfiguredTarget mop-up.
pkgValue = (PackageValue) aliasPackageValues.get(packageKey);
if (pkgValue == null) {
// This is unexpected: on the second iteration, all packages should be present,
// since the configured targets that depend on them are present. But since that is
// not a guarantee Skyframe makes, we tolerate their absence.
missedValues = true;
continue;
}
}
try {
BuildConfigurationValue depConfiguration = dep.getConfiguration();
BuildConfigurationKey depKey = depValue.getConfiguredTarget().getConfigurationKey();
if (depKey != null && !depKey.equals(depConfiguration.getKey())) {
depConfiguration = (BuildConfigurationValue) env.getValue(depKey);
}
result.put(
key,
new ConfiguredTargetAndData(
depValue.getConfiguredTarget(),
pkgValue.getPackage().getTarget(depLabel.getName()),
depConfiguration,
dep.getTransitionKeys()));
} catch (NoSuchTargetException e) {
throw new IllegalStateException("Target already verified for " + dep, e);
}
if (transitivePackagesForPackageRootResolution != null) {
transitivePackagesForPackageRootResolution.addTransitive(
depValue.getTransitivePackagesForPackageRootResolution());
}
}
depValue = (ConfiguredTargetValue) depValuesOrExceptions.get(key).get();
} catch (ConfiguredValueCreationException e) {
transitiveRootCauses.addTransitive(e.getRootCauses());
detailedExitCode =
Expand All @@ -917,24 +860,82 @@ private static Map<SkyKey, ConfiguredTargetAndData> resolveConfiguredTargetDepen
if (e.getDetailedExitCode().equals(detailedExitCode)) {
rootError = e;
}
continue;
}
if (depValue == null) {
missedValues = true;
continue;
}

ConfiguredTarget depCt = depValue.getConfiguredTarget();
Label depLabel = depCt.getLabel();
SkyKey packageKey = PackageValue.key(depLabel.getPackageIdentifier());
PackageValue pkgValue;
if (i == 0) {
ValueOrUntypedException packageResult = depValuesOrExceptions.get(packageKey);
if (packageResult == null) {
aliasPackagesToFetch.add(packageKey);
aliasDepsToRedo.add(dep);
continue;
} else {
pkgValue = (PackageValue) packageResult.getUnchecked();
if (pkgValue == null) {
// In a race, the getValuesOrThrow call above may have retrieved the package before it
// was done but the configured target after it was done. Since SkyFunctionEnvironment
// may cache absent values, re-requesting it on this evaluation may be useless, just
// treat it as missing.
missedValues = true;
continue;
}
}
} else {
// We were doing AliasConfiguredTarget mop-up.
pkgValue = (PackageValue) aliasPackageValues.get(packageKey);
if (pkgValue == null) {
// This is unexpected: on the second iteration, all packages should be present, since
// the configured targets that depend on them are present. But since that is not a
// guarantee Skyframe makes, we tolerate their absence.
missedValues = true;
continue;
}
}

try {
BuildConfigurationValue depConfiguration = dep.getConfiguration();
BuildConfigurationKey depKey = depValue.getConfiguredTarget().getConfigurationKey();
if (depKey != null && !depKey.equals(depConfiguration.getKey())) {
depConfiguration = (BuildConfigurationValue) env.getValue(depKey);
}
result.put(
key,
new ConfiguredTargetAndData(
depValue.getConfiguredTarget(),
pkgValue.getPackage().getTarget(depLabel.getName()),
depConfiguration,
dep.getTransitionKeys()));
} catch (NoSuchTargetException e) {
throw new IllegalStateException("Target already verified for " + dep, e);
}
if (transitivePackagesForPackageRootResolution != null) {
transitivePackagesForPackageRootResolution.addTransitive(
depValue.getTransitivePackagesForPackageRootResolution());
}
}

if (aliasDepsToRedo.isEmpty()) {
break;
}
aliasPackageValues = env.getValues(aliasPackagesToFetch);
depsToProcess = aliasDepsToRedo;
}

if (rootError != null) {
throw new DependencyEvaluationException(
new ConfiguredValueCreationException(
ctgValue, rootError.getMessage(), transitiveRootCauses.build(), detailedExitCode),
/*depReportedOwnError=*/ true);
} else if (missedValues) {
return null;
} else {
return result;
}
return missedValues ? null : result;
}

@Override
Expand Down
26 changes: 9 additions & 17 deletions src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
import net.starlark.java.eval.StarlarkSemantics;

Expand Down Expand Up @@ -135,15 +134,11 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps(

SkyKey pkgLookupKey = PackageLookupValue.key(packageId);
SkyKey dirListingKey = DirectoryListingValue.key(rootedPath);
Map<
SkyKey,
ValueOrException2<
NoSuchPackageException, IOException>>
pkgLookupAndDirectoryListingDeps =
env.getValuesOrThrow(
ImmutableList.of(pkgLookupKey, dirListingKey),
NoSuchPackageException.class,
IOException.class);
List<ValueOrException2<NoSuchPackageException, IOException>> pkgLookupAndDirectoryListingDeps =
env.getOrderedValuesOrThrow(
ImmutableList.of(pkgLookupKey, dirListingKey),
NoSuchPackageException.class,
IOException.class);
if (env.valuesMissing()) {
return null;
}
Expand All @@ -152,7 +147,7 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps(
pkgLookupValue =
(PackageLookupValue)
Preconditions.checkNotNull(
pkgLookupAndDirectoryListingDeps.get(pkgLookupKey).get(),
pkgLookupAndDirectoryListingDeps.get(0).get(),
"%s %s %s",
rootedPath,
repositoryName,
Expand All @@ -167,7 +162,7 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps(
dirListingValue =
(DirectoryListingValue)
Preconditions.checkNotNull(
pkgLookupAndDirectoryListingDeps.get(dirListingKey).get(),
pkgLookupAndDirectoryListingDeps.get(1).getOrThrow(IOException.class),
"%s %s %s",
rootedPath,
repositoryName,
Expand All @@ -177,12 +172,10 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps(
// but FileFunction was evaluated for rootedPath above, and didn't throw there. It shouldn't
// be able to avoid throwing there but throw here.
throw new IllegalStateException(
"Symlink cycle found after not being found for \"" + rootedPath + "\"");
"Symlink cycle found after not being found for \"" + rootedPath + "\"", e);
} catch (IOException e) {
return reportErrorAndReturn(
"Failed to list directory contents", e, rootRelativePath, env.getListener());
} catch (NoSuchPackageException e) {
throw new IllegalStateException(e);
}
StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
if (env.valuesMissing()) {
Expand All @@ -196,8 +189,7 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps(
repositoryName,
excludedPaths,
starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT)),
/** additionalValuesToAggregate= */
ImmutableMap.of());
/*additionalValuesToAggregate=*/ ImmutableMap.of());
}

private Iterable<SkyKey> getSubdirDeps(
Expand Down
48 changes: 42 additions & 6 deletions src/main/java/com/google/devtools/build/skyframe/ValueOrUntypedException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,53 @@
// limitations under the License.
package com.google.devtools.build.skyframe;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Throwables;
import javax.annotation.Nullable;

/**
* Wrapper for a value or the untyped exception thrown when trying to compute it.
*
* <p>This is an implementation detail of {@link ParallelEvaluator}. It's an abstract class (as
* opposed to an interface) to avoid exposing the methods outside the package.
* <p>Sky functions should generally prefer using exception-typed subclasses such as {@link
* ValueOrException}, {@link ValueOrException2}, etc. Occasionally, it may be useful to work with
* {@link ValueOrUntypedException} instead, for example to share logic for processing Skyframe
* requests that use different exception types or arity.
*/
@VisibleForTesting
public abstract class ValueOrUntypedException {

/**
* Returns the stored value, or {@code null} if it has not yet been computed. If an exception is
* stored, it is wrapped in {@link IllegalStateException} and thrown.
*
* <p>This method is useful when a value is not expected to store an exception but was requested
* in the same batch as values which may store an exception.
*/
@Nullable
public final SkyValue getUnchecked() {
Exception e = getException();
if (e != null) {
throw new IllegalStateException(e);
}
return getValue();
}

/**
* Returns the stored value, or {@code null} if it has not yet been computed. If an exception is
* stored, it is thrown as-is if it is an instance of {@code exceptionClass} and wrapped in {@link
* IllegalStateException} otherwise.
*
* <p>This method is useful when a value is only expected to store {@code exceptionClass} but was
* requested in the same batch as values which may store other types of exceptions.
*/
@Nullable
public final <E extends Exception> SkyValue getOrThrow(Class<E> exceptionClass) throws E {
Exception e = getException();
if (e != null) {
Throwables.throwIfInstanceOf(e, exceptionClass);
throw new IllegalStateException(e);
}
return getValue();
}

/** Returns the stored value, if there was one. */
@Nullable
abstract SkyValue getValue();
Expand All @@ -44,7 +80,7 @@ public static ValueOrUntypedException ofExn(Exception e) {
return new ValueOrUntypedExceptionExnImpl(e);
}

private static class ValueOrUntypedExceptionImpl extends ValueOrUntypedException {
private static final class ValueOrUntypedExceptionImpl extends ValueOrUntypedException {
static final ValueOrUntypedExceptionImpl NULL = new ValueOrUntypedExceptionImpl(null);
@Nullable private final SkyValue value;

Expand All @@ -64,7 +100,7 @@ public Exception getException() {
}
}

private static class ValueOrUntypedExceptionExnImpl extends ValueOrUntypedException {
private static final class ValueOrUntypedExceptionExnImpl extends ValueOrUntypedException {
private final Exception exception;

ValueOrUntypedExceptionExnImpl(Exception exception) {
Expand Down

0 comments on commit 0a93c1f

Please sign in to comment.