Try to clean up some of the excess baggage Aspects are carting around, mainly from a code cleanliness perspective, but with some minor memory benefits:
1. Use AspectKey instead of AspectValue wherever possible at the top-level, since keys are what callers should be using, not values.
2. Remove the freestanding label field from AspectKey. It is always equal to its ConfiguredTargetKey field's label. Having two fields with redundant data is confusing.
3. Remove the freestanding label field from AspectValue. Same reason: it was always the AspectKey's label.
4. Remove AspectDescriptor from ConfiguredAspect: it comes from the AspectKey, so just use the key.
This is also good because I think maximizing correspondence between ConfiguredAspect and ConfiguredTarget is a goal. Having an AspectValue muddies the waters. Note that, for historical reasons, ConfiguredTarget has much more data in it than we'd want a ConfiguredAspect to have (ideally, both would just have actions and providers).
In a follow-up, I may wrap AspectValue together with an AspectKey in TopLevelSkylarkAspectFunction#compute's return value. I think that, plus some tolerable casting, would remove the only remaining real need for AspectValue to have an AspectKey inside it.
PiperOrigin-RevId: 307877866
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletionValue.java
index e793177..1e7b315 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletionValue.java
@@ -15,7 +15,6 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.Iterables;
-import com.google.devtools.build.lib.analysis.AspectValue;
import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
import com.google.devtools.build.lib.skyframe.CompletionFunction.TopLevelActionLookupKey;
@@ -33,10 +32,8 @@
private AspectCompletionValue() {}
- public static Iterable<SkyKey> keys(
- Collection<AspectValue> targets, final TopLevelArtifactContext ctx) {
- return Iterables.transform(
- targets, aspectValue -> AspectCompletionKey.create(aspectValue.getKey(), ctx));
+ public static Iterable<SkyKey> keys(Collection<AspectKey> keys, TopLevelArtifactContext ctx) {
+ return Iterables.transform(keys, k -> AspectCompletionKey.create(k, ctx));
}
/** The key of an AspectCompletionValue. */
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletor.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletor.java
index b3c6455..2f5a251 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletor.java
@@ -18,7 +18,6 @@
import com.google.devtools.build.lib.actions.MissingInputFileException;
import com.google.devtools.build.lib.analysis.AspectCompleteEvent;
import com.google.devtools.build.lib.analysis.AspectValue;
-import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsToBuild;
import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
@@ -27,38 +26,43 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.skyframe.AspectCompletionValue.AspectCompletionKey;
+import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
import com.google.devtools.build.lib.skyframe.CompletionFunction.Completor;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
-import com.google.devtools.build.skyframe.SkyKey;
import java.util.function.Supplier;
import javax.annotation.Nullable;
/** Manages completing builds for aspects. */
-class AspectCompletor implements Completor<AspectValue, AspectCompletionValue> {
+class AspectCompletor
+ implements Completor<AspectValue, AspectCompletionValue, AspectCompletionKey> {
- public static SkyFunction aspectCompletionFunction(
+ static SkyFunction aspectCompletionFunction(
PathResolverFactory pathResolverFactory, Supplier<Path> execRootSupplier) {
return new CompletionFunction<>(pathResolverFactory, new AspectCompletor(), execRootSupplier);
}
@Override
- public Event getRootCauseError(AspectValue value, Cause rootCause, Environment env) {
+ public Event getRootCauseError(
+ AspectValue value, AspectCompletionKey key, Cause rootCause, Environment env) {
+ AspectKey aspectKey = key.actionLookupKey();
return Event.error(
value.getLocation(),
String.format(
"%s, aspect %s: missing input file '%s'",
- value.getLabel(), value.getConfiguredAspect().getName(), rootCause));
+ aspectKey.getLabel(), aspectKey.getAspectClass().getName(), rootCause));
}
@Override
public MissingInputFileException getMissingFilesException(
- AspectValue value, int missingCount, Environment env) {
+ AspectValue value, AspectCompletionKey key, int missingCount, Environment env) {
+ AspectKey aspectKey = key.actionLookupKey();
return new MissingInputFileException(
- value.getLabel()
+ aspectKey.getLabel()
+ ", aspect "
- + value.getConfiguredAspect().getName()
+ + aspectKey.getAspectClass().getName()
+ missingCount
+ " input file(s) do not exist",
value.getLocation());
@@ -75,9 +79,10 @@
NestedSet<Cause> rootCauses,
NestedSet<ArtifactsInOutputGroup> outputs,
Environment env,
- TopLevelArtifactContext topLevelArtifactContext)
+ AspectCompletionKey key)
throws InterruptedException {
- BuildEventId configurationEventId = getConfigurationEventIdFromAspectValue(value, env);
+ BuildEventId configurationEventId =
+ getConfigurationEventIdFromAspectKey(key.actionLookupKey(), env);
if (configurationEventId == null) {
return null;
}
@@ -85,14 +90,14 @@
}
@Nullable
- private BuildEventId getConfigurationEventIdFromAspectValue(AspectValue value, Environment env)
+ private BuildEventId getConfigurationEventIdFromAspectKey(AspectKey aspectKey, Environment env)
throws InterruptedException {
- if (value.getKey().getBaseConfiguredTargetKey().getConfigurationKey() == null) {
+ if (aspectKey.getBaseConfiguredTargetKey().getConfigurationKey() == null) {
return BuildEventIdUtil.nullConfigurationId();
} else {
BuildConfigurationValue buildConfigurationValue =
(BuildConfigurationValue)
- env.getValue(value.getKey().getBaseConfiguredTargetKey().getConfigurationKey());
+ env.getValue(aspectKey.getBaseConfiguredTargetKey().getConfigurationKey());
if (buildConfigurationValue == null) {
return null;
}
@@ -102,13 +107,14 @@
@Override
public ExtendedEventHandler.Postable createSucceeded(
- SkyKey skyKey,
+ AspectCompletionKey skyKey,
AspectValue value,
CompletionContext completionContext,
ArtifactsToBuild artifactsToBuild,
Environment env)
throws InterruptedException {
- BuildEventId configurationEventId = getConfigurationEventIdFromAspectValue(value, env);
+ AspectKey aspectKey = skyKey.actionLookupKey();
+ BuildEventId configurationEventId = getConfigurationEventIdFromAspectKey(aspectKey, env);
if (configurationEventId == null) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index 6dbeaa0..a15c5a1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -588,7 +588,6 @@
return new AspectValue(
originalKey,
aspect,
- originalTarget.getLabel(),
originalTarget.getLocation(),
ConfiguredAspect.forAlias(real.getConfiguredAspect()),
transitivePackagesForPackageRootResolution);
@@ -649,7 +648,7 @@
CurrentRuleTracker.endConfiguredAspect();
}
} else {
- configuredAspect = ConfiguredAspect.forNonapplicableTarget(aspect.getDescriptor());
+ configuredAspect = ConfiguredAspect.forNonapplicableTarget();
}
events.replayOn(env.getListener());
@@ -672,7 +671,6 @@
return new AspectValue(
key,
aspect,
- associatedTarget.getTarget().getLabel(),
associatedTarget.getTarget().getLocation(),
configuredAspect,
transitivePackagesForPackageRootResolution == null
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java
index 77ee261..bfd5266 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java
@@ -51,7 +51,6 @@
BuildConfiguration aspectConfiguration) {
KeyAndHost aspectKeyAndHost = ConfiguredTargetKey.keyFromConfiguration(aspectConfiguration);
return AspectKey.createAspectKey(
- label,
ConfiguredTargetKey.of(label, baseConfiguration),
baseKeys,
aspectDescriptor,
@@ -66,7 +65,6 @@
BuildConfiguration aspectConfiguration) {
KeyAndHost aspectKeyAndHost = ConfiguredTargetKey.keyFromConfiguration(aspectConfiguration);
return AspectKey.createAspectKey(
- label,
ConfiguredTargetKey.of(label, baseConfiguration),
ImmutableList.of(),
aspectDescriptor,
@@ -104,7 +102,6 @@
/** A base class for a key representing an aspect applied to a particular target. */
@AutoCodec
public static class AspectKey extends AspectValueKey {
- private final Label label;
private final ImmutableList<AspectKey> baseKeys;
private final BuildConfigurationValue.Key aspectConfigurationKey;
private final ConfiguredTargetKey baseConfiguredTargetKey;
@@ -112,13 +109,11 @@
private int hashCode;
private AspectKey(
- Label label,
BuildConfigurationValue.Key aspectConfigurationKey,
ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
AspectDescriptor aspectDescriptor) {
this.baseKeys = baseKeys;
- this.label = label;
this.aspectConfigurationKey = aspectConfigurationKey;
this.baseConfiguredTargetKey = baseConfiguredTargetKey;
this.aspectDescriptor = aspectDescriptor;
@@ -127,7 +122,6 @@
@AutoCodec.VisibleForSerialization
@AutoCodec.Instantiator
static AspectKey createAspectKey(
- Label label,
ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
AspectDescriptor aspectDescriptor,
@@ -136,17 +130,9 @@
return aspectKeyInterner.intern(
aspectConfigurationIsHost
? new HostAspectKey(
- label,
- aspectConfigurationKey,
- baseConfiguredTargetKey,
- baseKeys,
- aspectDescriptor)
+ aspectConfigurationKey, baseConfiguredTargetKey, baseKeys, aspectDescriptor)
: new AspectKey(
- label,
- aspectConfigurationKey,
- baseConfiguredTargetKey,
- baseKeys,
- aspectDescriptor));
+ aspectConfigurationKey, baseConfiguredTargetKey, baseKeys, aspectDescriptor));
}
@Override
@@ -157,7 +143,7 @@
@Override
public Label getLabel() {
- return label;
+ return baseConfiguredTargetKey.getLabel();
}
public AspectClass getAspectClass() {
@@ -249,7 +235,7 @@
private int computeHashCode() {
return Objects.hashCode(
- label, baseKeys, aspectConfigurationKey, baseConfiguredTargetKey, aspectDescriptor);
+ baseKeys, aspectConfigurationKey, baseConfiguredTargetKey, aspectDescriptor);
}
@Override
@@ -263,15 +249,14 @@
}
AspectKey that = (AspectKey) other;
- return Objects.equal(label, that.label)
- && Objects.equal(baseKeys, that.baseKeys)
+ return Objects.equal(baseKeys, that.baseKeys)
&& Objects.equal(aspectConfigurationKey, that.aspectConfigurationKey)
&& Objects.equal(baseConfiguredTargetKey, that.baseConfiguredTargetKey)
&& Objects.equal(aspectDescriptor, that.aspectDescriptor);
}
public String prettyPrint() {
- if (label == null) {
+ if (getLabel() == null) {
return "null";
}
@@ -281,7 +266,7 @@
: String.format(" (over %s)", baseKeys.toString());
return String.format(
"%s with aspect %s%s%s",
- label.toString(),
+ getLabel().toString(),
aspectDescriptor.getAspectClass().getName(),
(aspectConfigurationKey != null && aspectConfigurationIsHost()) ? "(host) " : "",
baseKeysString);
@@ -289,7 +274,7 @@
@Override
public String toString() {
- return (baseKeys == null ? label : baseKeys.toString())
+ return (baseKeys == null ? getLabel() : baseKeys.toString())
+ "#"
+ aspectDescriptor
+ " "
@@ -308,7 +293,6 @@
}
return createAspectKey(
- label,
ConfiguredTargetKey.of(
label,
baseConfiguredTargetKey.getConfigurationKey(),
@@ -323,12 +307,11 @@
/** An {@link AspectKey} for an aspect in the host configuration. */
static class HostAspectKey extends AspectKey {
private HostAspectKey(
- Label label,
BuildConfigurationValue.Key aspectConfigurationKey,
ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
AspectDescriptor aspectDescriptor) {
- super(label, aspectConfigurationKey, baseConfiguredTargetKey, baseKeys, aspectDescriptor);
+ super(aspectConfigurationKey, baseConfiguredTargetKey, baseKeys, aspectDescriptor);
}
@Override
@@ -453,7 +436,6 @@
AspectKey toAspectKey(AspectClass aspectClass) {
return AspectKey.createAspectKey(
- targetLabel,
baseConfiguredTargetKey,
ImmutableList.of(),
new AspectDescriptor(aspectClass, AspectParameters.EMPTY),
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/Builder.java b/src/main/java/com/google/devtools/build/lib/skyframe/Builder.java
index 753f611..0fb5828 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/Builder.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/Builder.java
@@ -14,12 +14,12 @@
package com.google.devtools.build.lib.skyframe;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Range;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.BuildFailedException;
import com.google.devtools.build.lib.actions.Executor;
import com.google.devtools.build.lib.actions.TestExecException;
-import com.google.devtools.build.lib.analysis.AspectValue;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
@@ -27,7 +27,6 @@
import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.common.options.OptionsProvider;
-import java.util.Collection;
import java.util.Set;
import javax.annotation.Nullable;
@@ -86,7 +85,7 @@
Set<ConfiguredTarget> exclusiveTests,
Set<ConfiguredTarget> targetsToBuild,
Set<ConfiguredTarget> targetsToSkip,
- Collection<AspectValue> aspects,
+ ImmutableSet<AspectKey> aspects,
Executor executor,
Set<ConfiguredTargetKey> builtTargets,
Set<AspectKey> builtAspects,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
index b1539d5..3002c8b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
@@ -24,6 +24,7 @@
import com.google.devtools.build.lib.actions.CompletionContext.PathResolverFactory;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.actions.MissingInputFileException;
+import com.google.devtools.build.lib.analysis.ConfiguredObjectValue;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper;
@@ -54,11 +55,13 @@
/** CompletionFunction builds the artifactsToBuild collection of a {@link ConfiguredTarget}. */
public final class CompletionFunction<
- ValueT extends ConfiguredObjectValue, ResultT extends SkyValue>
+ ValueT extends ConfiguredObjectValue,
+ ResultT extends SkyValue,
+ KeyT extends CompletionFunction.TopLevelActionLookupKey>
implements SkyFunction {
/** A strategy for completing the build. */
- interface Completor<ValueT, ResultT extends SkyValue> {
+ interface Completor<ValueT, ResultT extends SkyValue, KeyT extends TopLevelActionLookupKey> {
/**
* Returns the options which determine the artifacts to build for the top-level targets.
@@ -74,12 +77,12 @@
*/
/** Creates an event reporting an absent input artifact. */
- Event getRootCauseError(ValueT value, Cause rootCause, Environment env)
+ Event getRootCauseError(ValueT value, KeyT key, Cause rootCause, Environment env)
throws InterruptedException;
/** Creates an error message reporting {@code missingCount} missing input files. */
MissingInputFileException getMissingFilesException(
- ValueT value, int missingCount, Environment env) throws InterruptedException;
+ ValueT value, KeyT key, int missingCount, Environment env) throws InterruptedException;
/** Provides a successful completion value. */
ResultT getResult();
@@ -90,12 +93,12 @@
NestedSet<Cause> rootCauses,
NestedSet<ArtifactsInOutputGroup> outputs,
Environment env,
- TopLevelArtifactContext topLevelArtifactContext)
+ KeyT key)
throws InterruptedException;
/** Creates a succeeded completion value. */
ExtendedEventHandler.Postable createSucceeded(
- SkyKey skyKey,
+ KeyT skyKey,
ValueT value,
CompletionContext completionContext,
ArtifactsToBuild artifactsToBuild,
@@ -139,18 +142,19 @@
}
private final PathResolverFactory pathResolverFactory;
- private final Completor<ValueT, ResultT> completor;
+ private final Completor<ValueT, ResultT, KeyT> completor;
private final Supplier<Path> execRootSupplier;
CompletionFunction(
PathResolverFactory pathResolverFactory,
- Completor<ValueT, ResultT> completor,
+ Completor<ValueT, ResultT, KeyT> completor,
Supplier<Path> execRootSupplier) {
this.pathResolverFactory = pathResolverFactory;
this.completor = completor;
this.execRootSupplier = execRootSupplier;
}
+ @SuppressWarnings("unchecked") // Cast to KeyT
@Nullable
@Override
public SkyValue compute(SkyKey skyKey, Environment env)
@@ -161,7 +165,7 @@
return null;
}
- TopLevelActionLookupKey key = (TopLevelActionLookupKey) skyKey;
+ KeyT key = (KeyT) skyKey;
Pair<ValueT, ArtifactsToBuild> valueAndArtifactsToBuild = getValueAndArtifactsToBuild(key, env);
if (env.valuesMissing()) {
return null;
@@ -197,7 +201,7 @@
env.getListener().handle(Event.error(e.getLocation(), e.getMessage()));
Cause cause = new LabelCause(inputOwner, e.getMessage());
rootCausesBuilder.add(cause);
- env.getListener().handle(completor.getRootCauseError(value, cause, env));
+ env.getListener().handle(completor.getRootCauseError(value, key, cause, env));
}
} else {
builtArtifactsBuilder.add(input);
@@ -223,7 +227,7 @@
expandedFilesets.putAll(topLevelFilesets);
if (missingCount > 0) {
- missingInputException = completor.getMissingFilesException(value, missingCount, env);
+ missingInputException = completor.getMissingFilesException(value, key, missingCount, env);
if (missingInputException == null) {
return null;
}
@@ -236,8 +240,7 @@
builtArtifactsBuilder.build(), artifactsToBuild);
ExtendedEventHandler.Postable postable =
- completor.createFailed(
- value, rootCauses, builtOutputs, env, key.topLevelArtifactContext());
+ completor.createFailed(value, rootCauses, builtOutputs, env, key);
if (postable == null) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredObjectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredObjectValue.java
deleted file mode 100644
index 0141683..0000000
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredObjectValue.java
+++ /dev/null
@@ -1,48 +0,0 @@
-// Copyright 2020 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.skyframe;
-
-import com.google.devtools.build.lib.actions.ActionLookupValue;
-import com.google.devtools.build.lib.analysis.ProviderCollection;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
-import com.google.devtools.build.lib.packages.Package;
-import com.google.devtools.build.skyframe.NotComparableSkyValue;
-
-/**
- * Super-interface for {@link ConfiguredTargetValue} and {@link
- * com.google.devtools.build.lib.analysis.AspectValue}.
- */
-// TODO(janakr): Move this to analysis.
-public interface ConfiguredObjectValue extends ActionLookupValue, NotComparableSkyValue {
- /** Returns the configured target/aspect for this value. */
- ProviderCollection getConfiguredObject();
-
- /**
- * Returns the set of packages transitively loaded by this value. Must only be used for
- * constructing the package -> source root map needed for some builds. If the caller has not
- * specified that this map needs to be constructed (via the constructor argument in {@link
- * ConfiguredTargetFunction#ConfiguredTargetFunction} or {@link AspectFunction#AspectFunction}),
- * calling this will crash.
- */
- NestedSet<Package> getTransitivePackagesForPackageRootResolution();
-
- /**
- * Clears provider data from this value, leaving only the artifact->generating action map.
- *
- * <p>Should only be used when user specifies --discard_analysis_cache. Must be called at most
- * once per value, after which {@link #getConfiguredObject} and {@link #getActions} cannot be
- * called.
- */
- void clear(boolean clearEverything);
-}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetValue.java
index b65aa13..c0f8f77 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetValue.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.skyframe;
+import com.google.devtools.build.lib.analysis.ConfiguredObjectValue;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
/**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index f138d57..cbec840 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -1116,12 +1116,10 @@
return Preconditions.checkNotNull(
(GlobValue) valueOrException.get(), "%s should not be missing", globKey)
.getMatches();
- } catch (BuildFileNotFoundException e) {
+ } catch (BuildFileNotFoundException | IOException e) {
// Legacy package loading is only able to handle an IOException, so a rethrow here is the
// best we can do.
throw new SkyframeGlobbingIOException(e);
- } catch (IOException e) {
- throw new SkyframeGlobbingIOException(e);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index 22397e4..c74cde7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -671,7 +671,7 @@
@Override
public void clearAnalysisCache(
- Collection<ConfiguredTarget> topLevelTargets, Collection<AspectValue> topLevelAspects) {
+ Collection<ConfiguredTarget> topLevelTargets, ImmutableSet<AspectKey> topLevelAspects) {
discardPreExecutionCache(
topLevelTargets,
topLevelAspects,
@@ -810,7 +810,7 @@
actionGraphDump.dumpConfiguredTarget((ConfiguredTargetValue) skyValue);
} else if (functionName.equals(SkyFunctions.ASPECT)) {
AspectValue aspectValue = (AspectValue) skyValue;
- AspectKey aspectKey = aspectValue.getKey();
+ AspectKey aspectKey = (AspectKey) key;
ConfiguredTargetValue configuredTargetValue =
(ConfiguredTargetValue)
memoizingEvaluator.getExistingValue(aspectKey.getBaseConfiguredTargetKey());
@@ -842,7 +842,7 @@
actionGraphDump.dumpConfiguredTarget((ConfiguredTargetValue) skyValue);
} else if (functionName.equals(SkyFunctions.ASPECT)) {
AspectValue aspectValue = (AspectValue) skyValue;
- AspectKey aspectKey = aspectValue.getKey();
+ AspectKey aspectKey = (AspectKey) key;
ConfiguredTargetValue configuredTargetValue =
(ConfiguredTargetValue)
memoizingEvaluator.getExistingValue(aspectKey.getBaseConfiguredTargetKey());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java
index 75bda41..47e8806 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java
@@ -14,11 +14,12 @@
package com.google.devtools.build.lib.skyframe;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.PackageRoots;
-import com.google.devtools.build.lib.analysis.AspectValue;
+import com.google.devtools.build.lib.analysis.ConfiguredAspect;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
import com.google.devtools.build.skyframe.WalkableGraph;
-import java.util.Collection;
/**
* Encapsulates the raw analysis result of top level targets and aspects coming from Skyframe.
@@ -29,7 +30,7 @@
private final boolean hasActionConflicts;
private final ImmutableList<ConfiguredTarget> configuredTargets;
private final WalkableGraph walkableGraph;
- private final ImmutableList<AspectValue> aspects;
+ private final ImmutableMap<AspectKey, ConfiguredAspect> aspects;
private final PackageRoots packageRoots;
SkyframeAnalysisResult(
@@ -38,7 +39,7 @@
boolean hasActionConflicts,
ImmutableList<ConfiguredTarget> configuredTargets,
WalkableGraph walkableGraph,
- ImmutableList<AspectValue> aspects,
+ ImmutableMap<AspectKey, ConfiguredAspect> aspects,
PackageRoots packageRoots) {
this.hasLoadingError = hasLoadingError;
this.hasAnalysisError = hasAnalysisError;
@@ -74,7 +75,7 @@
return walkableGraph;
}
- public Collection<AspectValue> getAspects() {
+ public ImmutableMap<AspectKey, ConfiguredAspect> getAspects() {
return aspects;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 1fe6951..98bc456 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -40,6 +40,7 @@
import com.google.devtools.build.lib.analysis.AspectValue;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
+import com.google.devtools.build.lib.analysis.ConfiguredAspect;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.ConfiguredTargetFactory;
@@ -76,6 +77,7 @@
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.ConflictException;
+import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Root;
@@ -359,7 +361,7 @@
* @see com.google.devtools.build.lib.analysis.AnalysisOptions#discardAnalysisCache
*/
public void clearAnalysisCache(
- Collection<ConfiguredTarget> topLevelTargets, Collection<AspectValue> topLevelAspects) {
+ Collection<ConfiguredTarget> topLevelTargets, ImmutableSet<AspectKey> topLevelAspects) {
// TODO(bazel-team): Consider clearing packages too to save more memory.
skyframeAnalysisWasDiscarded = true;
skyframeExecutor.clearAnalysisCache(topLevelTargets, topLevelAspects);
@@ -391,7 +393,7 @@
enableAnalysis(false);
}
- Collection<AspectValue> aspects = Lists.newArrayListWithCapacity(aspectKeys.size());
+ Map<AspectKey, ConfiguredAspect> aspects = Maps.newHashMapWithExpectedSize(aspectKeys.size());
Root singleSourceRoot = skyframeExecutor.getForcedSingleSourceRootIfNoExecrootSymlinkCreation();
NestedSetBuilder<Package> packages =
singleSourceRoot == null ? NestedSetBuilder.stableOrder() : null;
@@ -401,7 +403,7 @@
// Skip aspects that couldn't be applied to targets.
continue;
}
- aspects.add(value);
+ aspects.put(value.getKey(), value.getConfiguredAspect());
if (packages != null) {
packages.addTransitive(value.getTransitivePackagesForPackageRootResolution());
}
@@ -453,7 +455,7 @@
foundActionConflict,
ImmutableList.copyOf(cts),
result.getWalkableGraph(),
- ImmutableList.copyOf(aspects),
+ ImmutableMap.copyOf(aspects),
packageRoots);
}
@@ -525,8 +527,11 @@
aspects =
aspectKeys.stream()
.filter(errorFreePredicate)
- .map(k -> Preconditions.checkNotNull((AspectValue) result.get(k), k))
- .collect(toImmutableList());
+ .map(result::get)
+ .map(AspectValue.class::cast)
+ .collect(
+ ImmutableMap.toImmutableMap(
+ AspectValue::getKey, AspectValue::getConfiguredAspect));
}
return new SkyframeAnalysisResult(
@@ -535,7 +540,7 @@
foundActionConflict,
ImmutableList.copyOf(cts),
result.getWalkableGraph(),
- ImmutableList.copyOf(aspects),
+ ImmutableMap.copyOf(aspects),
packageRoots);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 9d53ba4..915f58a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -142,6 +142,7 @@
import com.google.devtools.build.lib.runtime.KeepGoingOption;
import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
+import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
import com.google.devtools.build.lib.skyframe.DirtinessCheckerUtils.FileDirtinessChecker;
import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
import com.google.devtools.build.lib.skyframe.FileFunction.NonexistentFileReceiver;
@@ -1013,7 +1014,7 @@
*/
protected void discardPreExecutionCache(
Collection<ConfiguredTarget> topLevelTargets,
- Collection<AspectValue> topLevelAspects,
+ ImmutableSet<AspectKey> topLevelAspects,
DiscardType discardType) {
if (discardType.discardsAnalysis()) {
topLevelTargets = ImmutableSet.copyOf(topLevelTargets);
@@ -1076,7 +1077,7 @@
}
// value may be null if target was not successfully analyzed.
if (aspectValue != null) {
- aspectValue.clear(!topLevelAspects.contains(aspectValue));
+ aspectValue.clear(!topLevelAspects.contains(key));
}
}
}
@@ -1090,7 +1091,7 @@
* graph.
*/
public abstract void clearAnalysisCache(
- Collection<ConfiguredTarget> topLevelTargets, Collection<AspectValue> topLevelAspects);
+ Collection<ConfiguredTarget> topLevelTargets, ImmutableSet<AspectKey> topLevelAspects);
protected abstract void dropConfiguredTargetsNow(final ExtendedEventHandler eventHandler);
@@ -1564,7 +1565,7 @@
Executor executor,
Set<Artifact> artifactsToBuild,
Collection<ConfiguredTarget> targetsToBuild,
- Collection<AspectValue> aspects,
+ ImmutableSet<AspectKey> aspects,
Set<ConfiguredTarget> parallelTests,
Set<ConfiguredTarget> exclusiveTests,
OptionsProvider options,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index 09cc299..1d152ec 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -45,10 +45,9 @@
/** A factory for aspects that are defined in Starlark. */
public class SkylarkAspectFactory implements ConfiguredAspectFactory {
-
private final SkylarkDefinedAspect skylarkAspect;
- public SkylarkAspectFactory(SkylarkDefinedAspect skylarkAspect) {
+ SkylarkAspectFactory(SkylarkDefinedAspect skylarkAspect) {
this.skylarkAspect = skylarkAspect;
}
@@ -111,7 +110,7 @@
EvalUtils.getDataTypeName(aspectSkylarkObject)));
return null;
}
- return createAspect(aspectSkylarkObject, aspectDescriptor, ruleContext);
+ return createAspect(aspectSkylarkObject, ruleContext);
} catch (EvalException e) {
addAspectToStackTrace(ctadBase.getTarget(), e);
ruleContext.ruleError("\n" + e.print());
@@ -124,11 +123,10 @@
}
}
- private ConfiguredAspect createAspect(
- Object aspectSkylarkObject, AspectDescriptor aspectDescriptor, RuleContext ruleContext)
+ private static ConfiguredAspect createAspect(Object aspectSkylarkObject, RuleContext ruleContext)
throws EvalException, ActionConflictException {
- ConfiguredAspect.Builder builder = new ConfiguredAspect.Builder(aspectDescriptor, ruleContext);
+ ConfiguredAspect.Builder builder = new ConfiguredAspect.Builder(ruleContext);
if (aspectSkylarkObject instanceof Iterable) {
addDeclaredProviders(builder, (Iterable) aspectSkylarkObject);
@@ -165,7 +163,7 @@
return configuredAspect;
}
- private void addDeclaredProviders(
+ private static void addDeclaredProviders(
ConfiguredAspect.Builder builder, Iterable<?> aspectSkylarkObject) throws EvalException {
int i = 0;
for (Object o : aspectSkylarkObject) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletor.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletor.java
index 26611d9..76ad043 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletor.java
@@ -18,7 +18,6 @@
import com.google.devtools.build.lib.actions.MissingInputFileException;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.TargetCompleteEvent;
-import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsToBuild;
import com.google.devtools.build.lib.causes.Cause;
@@ -30,33 +29,33 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
-import com.google.devtools.build.skyframe.SkyKey;
import java.util.function.Supplier;
import javax.annotation.Nullable;
/** Manages completing builds for configured targets. */
-class TargetCompletor implements Completor<ConfiguredTargetValue, TargetCompletionValue> {
-
- public static SkyFunction targetCompletionFunction(
+class TargetCompletor
+ implements Completor<ConfiguredTargetValue, TargetCompletionValue, TargetCompletionKey> {
+ static SkyFunction targetCompletionFunction(
PathResolverFactory pathResolverFactory, Supplier<Path> execRootSupplier) {
return new CompletionFunction<>(pathResolverFactory, new TargetCompletor(), execRootSupplier);
}
@Override
- public Event getRootCauseError(ConfiguredTargetValue ctValue, Cause rootCause, Environment env)
+ public Event getRootCauseError(
+ ConfiguredTargetValue ctValue, TargetCompletionKey key, Cause rootCause, Environment env)
throws InterruptedException {
ConfiguredTargetAndData configuredTargetAndData =
ConfiguredTargetAndData.fromConfiguredTargetInSkyframe(ctValue.getConfiguredTarget(), env);
return Event.error(
configuredTargetAndData == null ? null : configuredTargetAndData.getTarget().getLocation(),
- String.format(
- "%s: missing input file '%s'", ctValue.getConfiguredTarget().getLabel(), rootCause));
+ String.format("%s: missing input file '%s'", key.actionLookupKey().getLabel(), rootCause));
}
@Override
@Nullable
public MissingInputFileException getMissingFilesException(
- ConfiguredTargetValue value, int missingCount, Environment env) throws InterruptedException {
+ ConfiguredTargetValue value, TargetCompletionKey key, int missingCount, Environment env)
+ throws InterruptedException {
ConfiguredTargetAndData configuredTargetAndData =
ConfiguredTargetAndData.fromConfiguredTargetInSkyframe(value.getConfiguredTarget(), env);
if (configuredTargetAndData == null) {
@@ -82,7 +81,7 @@
NestedSet<Cause> rootCauses,
NestedSet<ArtifactsInOutputGroup> outputs,
Environment env,
- TopLevelArtifactContext topLevelArtifactContext)
+ TargetCompletionKey key)
throws InterruptedException {
ConfiguredTarget target = value.getConfiguredTarget();
ConfiguredTargetAndData configuredTargetAndData =
@@ -96,7 +95,7 @@
@Override
@Nullable
public ExtendedEventHandler.Postable createSucceeded(
- SkyKey skyKey,
+ TargetCompletionKey skyKey,
ConfiguredTargetValue value,
CompletionContext completionContext,
ArtifactsToBuild artifactsToBuild,
@@ -108,7 +107,7 @@
if (configuredTargetAndData == null) {
return null;
}
- if (((TargetCompletionKey) skyKey.argument()).willTest()) {
+ if (skyKey.willTest()) {
return TargetCompleteEvent.successfulBuildSchedulingTest(
configuredTargetAndData,
completionContext,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupConflictFindingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupConflictFindingFunction.java
index 7f77c62..3ff17d9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupConflictFindingFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupConflictFindingFunction.java
@@ -17,6 +17,7 @@
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
+import com.google.devtools.build.lib.analysis.ConfiguredObjectValue;
import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper;
import com.google.devtools.build.lib.cmdline.Label;
