diff options
author | 2018-01-11 14:02:35 -0800 | |
---|---|---|
committer | 2018-01-11 14:04:37 -0800 | |
commit | 573807d4e9d1b7a8b6956278773dfc53b544093f (patch) | |
tree | ce89c3a760afbb113f78621a2b1ef0cbb9cef5e5 /src/main/java | |
parent | 42623f59fdd3bfbdfc21490c69f21537fa32011c (diff) |
Convert ActionLookupKey implementations to directly implement SkyKey, removing the layer of indirection of getting SkyKey out of ActionLookupKey, which uses more memory for no reason.
PiperOrigin-RevId: 181658615
Diffstat (limited to 'src/main/java')
30 files changed, 244 insertions, 209 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java index ffe650a19b..f1d91ba376 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java @@ -23,8 +23,6 @@ import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; -import com.google.devtools.build.skyframe.LegacySkyKey; -import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.util.ArrayList; @@ -151,11 +149,6 @@ public class ActionLookupValue implements SkyValue { return getStringHelper().toString(); } - @VisibleForTesting - public static SkyKey key(ActionLookupKey ownerKey) { - return ownerKey.getSkyKey(); - } - public int getNumActions() { return actions.size(); } @@ -190,43 +183,14 @@ public class ActionLookupValue implements SkyValue { } /** - * ArtifactOwner is not a SkyKey, but we wish to convert any ArtifactOwner into a SkyKey as simply - * as possible. To that end, all subclasses of ActionLookupValue "own" artifacts with - * ArtifactOwners that are subclasses of ActionLookupKey. This allows callers to easily find the - * value key, while remaining agnostic to what ActionLookupValues actually exist. - * - * <p>The methods of this class should only be called by {@link ActionLookupValue#key}. + * All subclasses of ActionLookupValue "own" artifacts with {@link ArtifactOwner}s that are + * subclasses of ActionLookupKey. This allows callers to easily find the value key, while + * remaining agnostic to what ActionLookupValues actually exist. */ - public abstract static class ActionLookupKey implements ArtifactOwner { + public abstract static class ActionLookupKey implements ArtifactOwner, SkyKey { @Override public Label getLabel() { return null; } - - /** - * Subclasses must override this to specify their specific value type, unless they override - * {@link #getSkyKey}, in which case they are free not to implement this method. - */ - protected abstract SkyFunctionName getType(); - - protected SkyKey getSkyKeyInternal() { - return LegacySkyKey.create(getType(), this); - } - - /** - * Prefer {@link ActionLookupValue#key} to calling this method directly. - * - * <p>Subclasses may override {@link #getSkyKeyInternal} if the {@link SkyKey} argument should - * not be this {@link ActionLookupKey} itself. - */ - public final SkyKey getSkyKey() { - SkyKey result = getSkyKeyInternal(); - Preconditions.checkState( - result.argument() instanceof ActionLookupKey, - "Not ActionLookupKey for %s: %s", - this, - result); - return result; - } } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java index d5e1f00998..35eeae8959 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java @@ -156,7 +156,7 @@ public final class AspectResolver { dependentAspects.build(), aspectDeps.getAspect(), dep.getAspectConfiguration(aspectDeps.getAspect())); - result.put(aspectKey.getAspectDescriptor(), aspectKey.getSkyKey()); + result.put(aspectKey.getAspectDescriptor(), aspectKey); return aspectKey; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 660f69b413..4b58054fda 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -490,13 +490,15 @@ public class BuildView { eventBus.post(new TargetConfiguredEvent(target, byLabel.get(target.getLabel()))); } - List<ConfiguredTargetKey> topLevelCtKeys = Lists.transform(topLevelTargetsWithConfigs, - new Function<TargetAndConfiguration, ConfiguredTargetKey>() { - @Override - public ConfiguredTargetKey apply(TargetAndConfiguration node) { - return new ConfiguredTargetKey(node.getLabel(), node.getConfiguration()); - } - }); + List<ConfiguredTargetKey> topLevelCtKeys = + Lists.transform( + topLevelTargetsWithConfigs, + new Function<TargetAndConfiguration, ConfiguredTargetKey>() { + @Override + public ConfiguredTargetKey apply(TargetAndConfiguration node) { + return ConfiguredTargetKey.of(node.getLabel(), node.getConfiguration()); + } + }); Multimap<Pair<Label, String>, BuildConfiguration> aspectConfigurations = ArrayListMultimap.create(); @@ -646,13 +648,14 @@ public class BuildView { Iterables.addAll(artifactsToBuild, baselineCoverageArtifacts); if (coverageReportActionFactory != null) { CoverageReportActionsWrapper actionsWrapper; - actionsWrapper = coverageReportActionFactory.createCoverageReportActionsWrapper( - eventHandler, - directories, - allTargetsToTest, - baselineCoverageArtifacts, - getArtifactFactory(), - CoverageReportValue.ARTIFACT_OWNER); + actionsWrapper = + coverageReportActionFactory.createCoverageReportActionsWrapper( + eventHandler, + directories, + allTargetsToTest, + baselineCoverageArtifacts, + getArtifactFactory(), + CoverageReportValue.COVERAGE_REPORT_KEY); if (actionsWrapper != null) { ImmutableList<ActionAnalysisMetadata> actions = actionsWrapper.getActions(); skyframeExecutor.injectCoverageReportData(actions); @@ -673,7 +676,7 @@ public class BuildView { public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) { ArtifactOwner artifactOwner = artifact.getArtifactOwner(); if (artifactOwner instanceof ActionLookupValue.ActionLookupKey) { - SkyKey key = ActionLookupValue.key((ActionLookupValue.ActionLookupKey) artifactOwner); + SkyKey key = (ActionLookupValue.ActionLookupKey) artifactOwner; ActionLookupValue val; try { val = (ActionLookupValue) graph.getValue(key); @@ -1081,7 +1084,7 @@ public class BuildView { new CachingAnalysisEnvironment( getArtifactFactory(), skyframeExecutor.getActionKeyContext(), - new ConfiguredTargetKey(target.getLabel(), targetConfig), + ConfiguredTargetKey.of(target.getLabel(), targetConfig), /*isSystemEnv=*/ false, targetConfig.extendedSanityChecks(), eventHandler, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java index 7d17950e2c..81d5d2e23f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java @@ -297,14 +297,14 @@ public class CachingAnalysisEnvironment implements AnalysisEnvironment { @Override public Artifact getStableWorkspaceStatusArtifact() throws InterruptedException { - return ((WorkspaceStatusValue) skyframeEnv.getValue(WorkspaceStatusValue.SKY_KEY)) - .getStableArtifact(); + return ((WorkspaceStatusValue) skyframeEnv.getValue(WorkspaceStatusValue.BUILD_INFO_KEY)) + .getStableArtifact(); } @Override public Artifact getVolatileWorkspaceStatusArtifact() throws InterruptedException { - return ((WorkspaceStatusValue) skyframeEnv.getValue(WorkspaceStatusValue.SKY_KEY)) - .getVolatileArtifact(); + return ((WorkspaceStatusValue) skyframeEnv.getValue(WorkspaceStatusValue.BUILD_INFO_KEY)) + .getVolatileArtifact(); } // See SkyframeBuildView#getWorkspaceStatusValues for the code that this method is attempting to @@ -327,8 +327,7 @@ public class CachingAnalysisEnvironment implements AnalysisEnvironment { throws InterruptedException { boolean stamp = AnalysisUtils.isStampingEnabled(ruleContext, config); BuildInfoCollectionValue collectionValue = - (BuildInfoCollectionValue) skyframeEnv.getValue(BuildInfoCollectionValue.key( - new BuildInfoCollectionValue.BuildInfoKeyAndConfig(key, config))); + (BuildInfoCollectionValue) skyframeEnv.getValue(BuildInfoCollectionValue.key(key, config)); if (collectionValue == null) { throw collectDebugInfoAndCrash(key, config); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index 25a33be726..913b6f1a71 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java @@ -166,8 +166,10 @@ public final class ConfiguredTargetFactory { Root root = rule.hasBinaryOutput() ? configuration.getBinDirectory(rule.getRepository()) : configuration.getGenfilesDirectory(rule.getRepository()); - ArtifactOwner owner = new ConfiguredTargetKey(rule.getLabel(), - getArtifactOwnerConfiguration(analysisEnvironment.getSkyframeEnv(), configuration)); + ArtifactOwner owner = + ConfiguredTargetKey.of( + rule.getLabel(), + getArtifactOwnerConfiguration(analysisEnvironment.getSkyframeEnv(), configuration)); if (analysisEnvironment.getSkyframeEnv().valuesMissing()) { return null; } @@ -271,11 +273,13 @@ public final class ConfiguredTargetFactory { } } else if (target instanceof InputFile) { InputFile inputFile = (InputFile) target; - Artifact artifact = artifactFactory.getSourceArtifact( - inputFile.getExecPath(), - Root.asSourceRoot(inputFile.getPackage().getSourceRoot(), - inputFile.getPackage().getPackageIdentifier().getRepository().isMain()), - new ConfiguredTargetKey(target.getLabel(), config)); + Artifact artifact = + artifactFactory.getSourceArtifact( + inputFile.getExecPath(), + Root.asSourceRoot( + inputFile.getPackage().getSourceRoot(), + inputFile.getPackage().getPackageIdentifier().getRepository().isMain()), + ConfiguredTargetKey.of(target.getLabel(), config)); return new InputFileConfiguredTarget(targetContext, inputFile, artifact); } else if (target instanceof PackageGroup) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetAndConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/TargetAndConfiguration.java index e64098d39e..21ed49ed98 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/TargetAndConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/TargetAndConfiguration.java @@ -13,13 +13,11 @@ // limitations under the License. package com.google.devtools.build.lib.analysis; -import com.google.common.base.Function; import com.google.common.base.Preconditions; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.packages.Target; -import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; import java.util.Objects; import javax.annotation.Nullable; @@ -49,22 +47,6 @@ public final class TargetAndConfiguration { + (configuration == null ? "null" : configuration.checksum()); } - public static final Function<TargetAndConfiguration, String> NAME_FUNCTION = - new Function<TargetAndConfiguration, String>() { - @Override - public String apply(TargetAndConfiguration node) { - return node.getName(); - } - }; - - public static final Function<TargetAndConfiguration, ConfiguredTargetKey> - TO_LABEL_AND_CONFIGURATION = new Function<TargetAndConfiguration, ConfiguredTargetKey>() { - @Override - public ConfiguredTargetKey apply(TargetAndConfiguration input) { - return new ConfiguredTargetKey(input.getLabel(), input.getConfiguration()); - } - }; - @Override public boolean equals(Object that) { if (this == that) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 4f7bce06a0..549341a872 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -474,7 +474,7 @@ public class CppCompileAction extends AbstractAction } Map<Artifact, SkyKey> skyKeys = new HashMap<>(); for (Artifact artifact : this.usedModules) { - skyKeys.put(artifact, ActionLookupValue.key((ActionLookupKey) artifact.getArtifactOwner())); + skyKeys.put(artifact, (ActionLookupKey) artifact.getArtifactOwner()); } Map<SkyKey, SkyValue> skyValues = env.getValues(skyKeys.values()); Set<Artifact> additionalModules = Sets.newLinkedHashSet(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java index 7771e08ce2..0897116f7b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java @@ -15,20 +15,19 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Interner; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.analysis.actions.ActionTemplate; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.skyframe.LegacySkyKey; +import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.skyframe.SkyFunctionName; -import com.google.devtools.build.skyframe.SkyKey; /** * Value that stores expanded actions from ActionTemplate. */ public final class ActionTemplateExpansionValue extends ActionLookupValue { - ActionTemplateExpansionValue( ActionKeyContext actionKeyContext, Iterable<Action> expandedActions, @@ -36,21 +35,17 @@ public final class ActionTemplateExpansionValue extends ActionLookupValue { super(actionKeyContext, ImmutableList.copyOf(expandedActions), removeActionsAfterEvaluation); } - static SkyKey key(ActionTemplate<?> actionTemplate) { - return LegacySkyKey.create( - SkyFunctions.ACTION_TEMPLATE_EXPANSION, createActionTemplateExpansionKey(actionTemplate)); - } + private static final Interner<ActionTemplateExpansionKey> interner = + BlazeInterners.newWeakInterner(); - static ActionTemplateExpansionKey createActionTemplateExpansionKey( - ActionTemplate<?> actionTemplate) { - return new ActionTemplateExpansionKey(actionTemplate); + static ActionTemplateExpansionKey key(ActionTemplate<?> actionTemplate) { + return interner.intern(new ActionTemplateExpansionKey(actionTemplate)); } - static final class ActionTemplateExpansionKey extends ActionLookupKey { private final ActionTemplate<?> actionTemplate; - ActionTemplateExpansionKey(ActionTemplate<?> actionTemplate) { + private ActionTemplateExpansionKey(ActionTemplate<?> actionTemplate) { Preconditions.checkNotNull( actionTemplate, "Passed in action template cannot be null: %s", @@ -59,11 +54,10 @@ public final class ActionTemplateExpansionValue extends ActionLookupValue { } @Override - protected SkyFunctionName getType() { + public SkyFunctionName functionName() { return SkyFunctions.ACTION_TEMPLATE_EXPANSION; } - @Override public Label getLabel() { return actionTemplate.getOwner().getLabel(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java index bb0ddbacd9..10016a1e5d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java @@ -315,7 +315,7 @@ class ArtifactFunction implements SkyFunction { ArtifactOwner artifactOwner = artifact.getArtifactOwner(); Preconditions.checkState(artifactOwner instanceof ActionLookupKey, "", artifact, artifactOwner); - return ActionLookupValue.key((ActionLookupKey) artifactOwner); + return (ActionLookupKey) artifactOwner; } @Nullable @@ -326,7 +326,7 @@ class ArtifactFunction implements SkyFunction { if (value == null) { ArtifactOwner artifactOwner = artifact.getArtifactOwner(); Preconditions.checkState( - artifactOwner == CoverageReportValue.ARTIFACT_OWNER, + artifactOwner == CoverageReportValue.COVERAGE_REPORT_KEY, "Not-yet-present artifact owner: %s (%s %s)", artifactOwner, artifact, 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 f3d47e3763..56599c1ab8 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 @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.ActionKeyContext; -import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.analysis.AliasProvider; import com.google.devtools.build.lib.analysis.AspectResolver; import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment; @@ -408,8 +407,7 @@ public final class AspectFunction implements SkyFunction { throws DuplicateException { ArrayList<ConfiguredAspect> aspectValues = new ArrayList<>(); for (AspectKey aspectKey : aspectKeys) { - SkyKey skyAspectKey = aspectKey.getSkyKey(); - AspectValue aspectValue = (AspectValue) values.get(skyAspectKey); + AspectValue aspectValue = (AspectValue) values.get(aspectKey); ConfiguredAspect configuredAspect = aspectValue.getConfiguredAspect(); aspectValues.add(configuredAspect); } @@ -438,14 +436,13 @@ public final class AspectFunction implements SkyFunction { return; } ImmutableList<AspectKey> baseKeys = key.getBaseKeys(); - SkyKey skyKey = key.getSkyKey(); - result.put(key.getAspectDescriptor(), skyKey); + result.put(key.getAspectDescriptor(), key); for (AspectKey baseKey : baseKeys) { buildSkyKeys(baseKey, result, aspectPathBuilder); } // Post-order list of aspect SkyKeys gives the order of propagating aspects: // the aspect comes after all aspects it transitively sees. - aspectPathBuilder.add(skyKey); + aspectPathBuilder.add(key); } private SkyValue createAliasAspect( @@ -462,7 +459,7 @@ public final class AspectFunction implements SkyFunction { // the real configured target. Label aliasLabel = aliasChain.size() > 1 ? aliasChain.get(1) : configuredTarget.getLabel(); - SkyKey depKey = ActionLookupValue.key(originalKey.withLabel(aliasLabel)); + SkyKey depKey = originalKey.withLabel(aliasLabel); // Compute the AspectValue of the target the alias refers to (which can itself be either an // alias or a real target) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java index a6ecc98a77..b6c6e0d366 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java @@ -17,6 +17,7 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Interner; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionLookupValue; @@ -24,6 +25,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredAspect; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; +import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.packages.Aspect; import com.google.devtools.build.lib.packages.AspectClass; @@ -56,6 +58,7 @@ public final class AspectValue extends ActionLookupValue { private final BuildConfiguration aspectConfiguration; private final BuildConfiguration baseConfiguration; private final AspectDescriptor aspectDescriptor; + private int hashCode; private AspectKey( Label label, @@ -71,7 +74,7 @@ public final class AspectValue extends ActionLookupValue { } @Override - protected SkyFunctionName getType() { + public SkyFunctionName functionName() { return SkyFunctions.ASPECT; } @@ -148,6 +151,31 @@ public final class AspectValue extends ActionLookupValue { @Override public int hashCode() { + // We use the hash code caching strategy employed by java.lang.String. There are three subtle + // things going on here: + // + // (1) We use a value of 0 to indicate that the hash code hasn't been computed and cached yet. + // Yes, this means that if the hash code is really 0 then we will "recompute" it each time. + // But this isn't a problem in practice since a hash code of 0 should be rare. + // + // (2) Since we have no synchronization, multiple threads can race here thinking there are the + // first one to compute and cache the hash code. + // + // (3) Moreover, since 'hashCode' is non-volatile, the cached hash code value written from one + // thread may not be visible by another. + // + // All three of these issues are benign from a correctness perspective; in the end we have no + // overhead from synchronization, at the cost of potentially computing the hash code more than + // once. + int h = hashCode; + if (h == 0) { + h = computeHashCode(); + hashCode = h; + } + return h; + } + + private int computeHashCode() { return Objects.hashCode( label, baseKeys, @@ -227,6 +255,7 @@ public final class AspectValue extends ActionLookupValue { private final BuildConfiguration targetConfiguration; private final SkylarkImport skylarkImport; private final String skylarkValueName; + private int hashCode; private SkylarkAspectLoadingKey( Label targetLabel, @@ -243,7 +272,7 @@ public final class AspectValue extends ActionLookupValue { } @Override - protected SkyFunctionName getType() { + public SkyFunctionName functionName() { return SkyFunctions.LOAD_SKYLARK_ASPECT; } @@ -282,6 +311,31 @@ public final class AspectValue extends ActionLookupValue { @Override public int hashCode() { + // We use the hash code caching strategy employed by java.lang.String. There are three subtle + // things going on here: + // + // (1) We use a value of 0 to indicate that the hash code hasn't been computed and cached yet. + // Yes, this means that if the hash code is really 0 then we will "recompute" it each time. + // But this isn't a problem in practice since a hash code of 0 should be rare. + // + // (2) Since we have no synchronization, multiple threads can race here thinking there are the + // first one to compute and cache the hash code. + // + // (3) Moreover, since 'hashCode' is non-volatile, the cached hash code value written from one + // thread may not be visible by another. + // + // All three of these issues are benign from a correctness perspective; in the end we have no + // overhead from synchronization, at the cost of potentially computing the hash code more than + // once. + int h = hashCode; + if (h == 0) { + h = computeHashCode(); + hashCode = h; + } + return h; + } + + private int computeHashCode() { return Objects.hashCode(targetLabel, aspectConfiguration, targetConfiguration, @@ -392,24 +446,27 @@ public final class AspectValue extends ActionLookupValue { ImmutableList<AspectKey> baseKeys, AspectDescriptor aspectDescriptor, BuildConfiguration aspectConfiguration) { - return new AspectKey( + return aspectKeyInterner.intern(new AspectKey( label, baseConfiguration, baseKeys, aspectDescriptor, aspectConfiguration - ); + )); } + private static final Interner<AspectKey> aspectKeyInterner = BlazeInterners.newWeakInterner(); public static AspectKey createAspectKey( Label label, BuildConfiguration baseConfiguration, AspectDescriptor aspectDescriptor, BuildConfiguration aspectConfiguration) { - return new AspectKey( - label, baseConfiguration, ImmutableList.<AspectKey>of(), - aspectDescriptor, aspectConfiguration); + return aspectKeyInterner.intern( + new AspectKey( + label, baseConfiguration, ImmutableList.of(), aspectDescriptor, aspectConfiguration)); } + private static final Interner<SkylarkAspectLoadingKey> skylarkAspectKeyInterner = + BlazeInterners.newWeakInterner(); public static SkylarkAspectLoadingKey createSkylarkAspectKey( Label targetLabel, @@ -417,7 +474,12 @@ public final class AspectValue extends ActionLookupValue { BuildConfiguration targetConfiguration, SkylarkImport skylarkImport, String skylarkExportName) { - return new SkylarkAspectLoadingKey( - targetLabel, aspectConfiguration, targetConfiguration, skylarkImport, skylarkExportName); + return skylarkAspectKeyInterner.intern( + new SkylarkAspectLoadingKey( + targetLabel, + aspectConfiguration, + targetConfiguration, + skylarkImport, + skylarkExportName)); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java index 62878a5fe2..c80875d7d8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java @@ -58,7 +58,7 @@ public class BuildInfoCollectionFunction implements SkyFunction { public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { final BuildInfoKeyAndConfig keyAndConfig = (BuildInfoKeyAndConfig) skyKey.argument(); WorkspaceStatusValue infoArtifactValue = - (WorkspaceStatusValue) env.getValue(WorkspaceStatusValue.SKY_KEY); + (WorkspaceStatusValue) env.getValue(WorkspaceStatusValue.BUILD_INFO_KEY); if (infoArtifactValue == null) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java index 64869399b5..8695042cca 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java @@ -14,12 +14,14 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Preconditions; +import com.google.common.collect.Interner; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoCollection; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.skyframe.SkyFunctionName; import java.util.Objects; @@ -47,18 +49,26 @@ public class BuildInfoCollectionValue extends ActionLookupValue { return getStringHelper().add("collection", collection).toString(); } + private static final Interner<BuildInfoKeyAndConfig> keyInterner = + BlazeInterners.newWeakInterner(); + + public static BuildInfoKeyAndConfig key( + BuildInfoFactory.BuildInfoKey key, BuildConfiguration config) { + return keyInterner.intern(new BuildInfoKeyAndConfig(key, config)); + } + /** Key for BuildInfoCollectionValues. */ public static class BuildInfoKeyAndConfig extends ActionLookupKey { private final BuildInfoFactory.BuildInfoKey infoKey; private final BuildConfiguration config; - public BuildInfoKeyAndConfig(BuildInfoFactory.BuildInfoKey key, BuildConfiguration config) { + private BuildInfoKeyAndConfig(BuildInfoFactory.BuildInfoKey key, BuildConfiguration config) { this.infoKey = Preconditions.checkNotNull(key, config); this.config = Preconditions.checkNotNull(config, key); } @Override - protected SkyFunctionName getType() { + public SkyFunctionName functionName() { return SkyFunctions.BUILD_INFO_COLLECTION; } 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 02bb53fd10..db8d44ccc2 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 @@ -103,8 +103,7 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S public ConfiguredTargetValue getValueFromSkyKey(SkyKey skyKey, Environment env) throws InterruptedException { TargetCompletionKey tcKey = (TargetCompletionKey) skyKey.argument(); - ConfiguredTargetKey lac = tcKey.configuredTargetKey(); - return (ConfiguredTargetValue) env.getValue(lac.getSkyKey()); + return (ConfiguredTargetValue) env.getValue(tcKey.configuredTargetKey()); } @Override @@ -179,7 +178,7 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S throws InterruptedException { AspectCompletionKey acKey = (AspectCompletionKey) skyKey.argument(); AspectKey aspectKey = acKey.aspectKey(); - return (AspectValue) env.getValue(aspectKey.getSkyKey()); + return (AspectValue) env.getValue(aspectKey); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index af83e3c9a6..258223b3f0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -604,9 +604,13 @@ public final class ConfiguredTargetFunction implements SkyFunction { if (env.valuesMissing()) { return null; } - CachingAnalysisEnvironment analysisEnvironment = view.createAnalysisEnvironment( - new ConfiguredTargetKey(target.getLabel(), ownerConfig), false, - events, env, configuration); + CachingAnalysisEnvironment analysisEnvironment = + view.createAnalysisEnvironment( + ConfiguredTargetKey.of(target.getLabel(), ownerConfig), + false, + events, + env, + configuration); if (env.valuesMissing()) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java index c02fd999d9..6cdd9599c6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java @@ -14,13 +14,14 @@ package com.google.devtools.build.lib.skyframe; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.collect.Interner; import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey; import com.google.devtools.build.lib.analysis.AliasProvider; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.skyframe.SkyFunctionName; import java.util.Objects; import javax.annotation.Nullable; @@ -36,16 +37,13 @@ public class ConfiguredTargetKey extends ActionLookupKey { @Nullable private final BuildConfiguration configuration; - public ConfiguredTargetKey(Label label, @Nullable BuildConfiguration configuration) { + private transient int hashCode; + + private ConfiguredTargetKey(Label label, @Nullable BuildConfiguration configuration) { this.label = Preconditions.checkNotNull(label); this.configuration = configuration; } - @VisibleForTesting - public ConfiguredTargetKey(ConfiguredTarget rule) { - this(rule.getTarget().getLabel(), rule.getConfiguration()); - } - public static ConfiguredTargetKey of(ConfiguredTarget configuredTarget) { AliasProvider aliasProvider = configuredTarget.getProvider(AliasProvider.class); Label label = @@ -53,8 +51,14 @@ public class ConfiguredTargetKey extends ActionLookupKey { return of(label, configuredTarget.getConfiguration()); } + /** + * Cache so that the number of ConfiguredTargetKey instances is {@code O(configured targets)} and + * not {@code O(edges between configured targets)}. + */ + private static final Interner<ConfiguredTargetKey> interner = BlazeInterners.newWeakInterner(); + public static ConfiguredTargetKey of(Label label, @Nullable BuildConfiguration configuration) { - return new ConfiguredTargetKey(label, configuration); + return interner.intern(new ConfiguredTargetKey(label, configuration)); } @Override @@ -63,7 +67,7 @@ public class ConfiguredTargetKey extends ActionLookupKey { } @Override - protected SkyFunctionName getType() { + public SkyFunctionName functionName() { return SkyFunctions.CONFIGURED_TARGET; } @@ -74,6 +78,31 @@ public class ConfiguredTargetKey extends ActionLookupKey { @Override public int hashCode() { + // We use the hash code caching strategy employed by java.lang.String. There are three subtle + // things going on here: + // + // (1) We use a value of 0 to indicate that the hash code hasn't been computed and cached yet. + // Yes, this means that if the hash code is really 0 then we will "recompute" it each time. But + // this isn't a problem in practice since a hash code of 0 should be rare. + // + // (2) Since we have no synchronization, multiple threads can race here thinking there are the + // first one to compute and cache the hash code. + // + // (3) Moreover, since 'hashCode' is non-volatile, the cached hash code value written from one + // thread may not be visible by another. + // + // All three of these issues are benign from a correctness perspective; in the end we have no + // overhead from synchronization, at the cost of potentially computing the hash code more than + // once. + int h = hashCode; + if (h == 0) { + h = computeHashCode(); + hashCode = h; + } + return h; + } + + private int computeHashCode() { int configVal = configuration == null ? 79 : configuration.hashCode(); return 31 * label.hashCode() + configVal; } 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 a11e27a092..fc4ebd6727 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 @@ -100,13 +100,13 @@ public final class ConfiguredTargetValue extends ActionLookupValue { @VisibleForTesting public static SkyKey key(Label label, BuildConfiguration configuration) { - return key(new ConfiguredTargetKey(label, configuration)); + return ConfiguredTargetKey.of(label, configuration); } static ImmutableList<SkyKey> keys(Iterable<ConfiguredTargetKey> lacs) { ImmutableList.Builder<SkyKey> keys = ImmutableList.builder(); for (ConfiguredTargetKey lac : lacs) { - keys.add(key(lac)); + keys.add(lac); } return keys.build(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java index 64fdcc4fb5..3fbf2d9e2d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java @@ -41,8 +41,10 @@ public class CoverageReportFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { Preconditions.checkState( - CoverageReportValue.SKY_KEY.equals(skyKey), String.format( - "Expected %s for SkyKey but got %s instead", CoverageReportValue.SKY_KEY, skyKey)); + CoverageReportValue.COVERAGE_REPORT_KEY.equals(skyKey), + String.format( + "Expected %s for SkyKey but got %s instead", + CoverageReportValue.COVERAGE_REPORT_KEY, skyKey)); ImmutableList<ActionAnalysisMetadata> actions = PrecomputedValue.COVERAGE_REPORT_KEY.get(env); if (actions == null) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java index fde0599749..fd9bace71e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java @@ -18,10 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionLookupValue; -import com.google.devtools.build.lib.actions.ArtifactOwner; -import com.google.devtools.build.skyframe.LegacySkyKey; import com.google.devtools.build.skyframe.SkyFunctionName; -import com.google.devtools.build.skyframe.SkyKey; /** * A SkyValue to store the coverage report Action and Artifacts. @@ -29,8 +26,7 @@ import com.google.devtools.build.skyframe.SkyKey; public class CoverageReportValue extends ActionLookupValue { // There should only ever be one CoverageReportValue value in the graph. - public static final ArtifactOwner ARTIFACT_OWNER = new CoverageReportKey(); - static final SkyKey SKY_KEY = LegacySkyKey.create(SkyFunctions.COVERAGE_REPORT, ARTIFACT_OWNER); + public static final CoverageReportKey COVERAGE_REPORT_KEY = new CoverageReportKey(); CoverageReportValue( ActionKeyContext actionKeyContext, @@ -39,15 +35,13 @@ public class CoverageReportValue extends ActionLookupValue { super(actionKeyContext, coverageReportActions, removeActionsAfterEvaluation); } - private static class CoverageReportKey extends ActionLookupKey { - @Override - protected SkyFunctionName getType() { - throw new UnsupportedOperationException(); - } + static class CoverageReportKey extends ActionLookupKey { + private CoverageReportKey() {} @Override - protected SkyKey getSkyKeyInternal() { - return SKY_KEY; + public SkyFunctionName functionName() { + return SkyFunctions.COVERAGE_REPORT; } + } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java index 779dbaa255..3ae57a524e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java @@ -51,12 +51,12 @@ import javax.annotation.Nullable; public class PostConfiguredTargetFunction implements SkyFunction { private static final Function<Dependency, SkyKey> TO_KEYS = new Function<Dependency, SkyKey>() { - @Override - public SkyKey apply(Dependency input) { - return PostConfiguredTargetValue.key( - new ConfiguredTargetKey(input.getLabel(), input.getConfiguration())); - } - }; + @Override + public SkyKey apply(Dependency input) { + return PostConfiguredTargetValue.key( + ConfiguredTargetKey.of(input.getLabel(), input.getConfiguration())); + } + }; private final SkyframeExecutor.BuildViewProvider buildViewProvider; private final RuleClassProvider ruleClassProvider; @@ -74,8 +74,8 @@ public class PostConfiguredTargetFunction implements SkyFunction { throws SkyFunctionException, InterruptedException { ImmutableMap<ActionAnalysisMetadata, ConflictException> badActions = PrecomputedValue.BAD_ACTIONS.get(env); - ConfiguredTargetValue ctValue = (ConfiguredTargetValue) - env.getValue(ConfiguredTargetValue.key((ConfiguredTargetKey) skyKey.argument())); + ConfiguredTargetValue ctValue = + (ConfiguredTargetValue) env.getValue((ConfiguredTargetKey) skyKey.argument()); if (env.valuesMissing()) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java index d8f7b1b46c..ad531b30f5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java @@ -106,7 +106,7 @@ public class RegisteredToolchainsFunction implements SkyFunction { label -> LegacySkyKey.create( SkyFunctions.CONFIGURED_TARGET, - new ConfiguredTargetKey(label, configuration))) + ConfiguredTargetKey.of(label, configuration))) .collect(ImmutableList.toImmutableList()); Map<SkyKey, ValueOrException<ConfiguredValueCreationException>> values = 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 8adef6d163..0313d477d8 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 @@ -743,7 +743,6 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { // well as ActionExecutionValues, since they do not depend on ActionLookupValues. SkyFunctionName.functionIsIn(ImmutableSet.of( SkyFunctions.CONFIGURED_TARGET, - SkyFunctions.ACTION_LOOKUP, SkyFunctions.BUILD_INFO, SkyFunctions.TARGET_COMPLETION, SkyFunctions.BUILD_INFO_COLLECTION, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index 4f7479b5d6..c701fa6177 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java @@ -86,7 +86,6 @@ public final class SkyFunctions { public static final SkyFunctionName ARTIFACT = SkyFunctionName.create("ARTIFACT"); public static final SkyFunctionName ACTION_EXECUTION = SkyFunctionName.create("ACTION_EXECUTION"); - public static final SkyFunctionName ACTION_LOOKUP = SkyFunctionName.create("ACTION_LOOKUP"); public static final SkyFunctionName RECURSIVE_FILESYSTEM_TRAVERSAL = SkyFunctionName.create("RECURSIVE_DIRECTORY_TRAVERSAL"); public static final SkyFunctionName FILESET_ENTRY = SkyFunctionName.create("FILESET_ENTRY"); 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 a62d501ccb..a137435872 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 @@ -60,7 +60,6 @@ import com.google.devtools.build.lib.pkgcache.LoadingFailureEvent; import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner; import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException; import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey; -import com.google.devtools.build.lib.skyframe.BuildInfoCollectionValue.BuildInfoKeyAndConfig; import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.ConfiguredValueCreationException; import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ConflictException; import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException; @@ -221,7 +220,7 @@ public final class SkyframeBuildView { NestedSetBuilder<Package> packages = singleSourceRoot == null ? NestedSetBuilder.stableOrder() : null; for (AspectValueKey aspectKey : aspectKeys) { - AspectValue value = (AspectValue) result.get(aspectKey.getSkyKey()); + AspectValue value = (AspectValue) result.get(aspectKey); if (value == null) { // Skip aspects that couldn't be applied to targets. continue; @@ -237,8 +236,7 @@ public final class SkyframeBuildView { // list of values, i.e., that the order is deterministic. Collection<ConfiguredTarget> goodCts = Lists.newArrayListWithCapacity(values.size()); for (ConfiguredTargetKey value : values) { - ConfiguredTargetValue ctValue = - (ConfiguredTargetValue) result.get(ConfiguredTargetValue.key(value)); + ConfiguredTargetValue ctValue = (ConfiguredTargetValue) result.get(value); if (ctValue == null) { continue; } @@ -450,13 +448,13 @@ public final class SkyframeBuildView { BuildConfiguration config, ImmutableMap<BuildInfoKey, BuildInfoFactory> buildInfoFactories) throws InterruptedException { - env.getValue(WorkspaceStatusValue.SKY_KEY); + env.getValue(WorkspaceStatusValue.BUILD_INFO_KEY); // These factories may each create their own build info artifacts, all depending on the basic // build-info.txt and build-changelist.txt. List<SkyKey> depKeys = Lists.newArrayList(); for (BuildInfoKey key : buildInfoFactories.keySet()) { if (buildInfoFactories.get(key).isEnabled(config)) { - depKeys.add(BuildInfoCollectionValue.key(new BuildInfoKeyAndConfig(key, config))); + depKeys.add(BuildInfoCollectionValue.key(key, config)); } } env.getValues(depKeys); 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 a59f8652af..429caf8344 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 @@ -752,20 +752,21 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { // TODO(janakr): don't invalidate here, just use different keys for different configs. Can't // be done right now because of lack of configuration trimming and fact that everything // depends on workspace status action. - invalidate(WorkspaceStatusValue.SKY_KEY::equals); + invalidate(WorkspaceStatusValue.BUILD_INFO_KEY::equals); } } private WorkspaceStatusAction makeWorkspaceStatusAction(String workspaceName) { return workspaceStatusActionFactory.createWorkspaceStatusAction( - artifactFactory.get(), WorkspaceStatusValue.ARTIFACT_OWNER, buildId, workspaceName); + artifactFactory.get(), WorkspaceStatusValue.BUILD_INFO_KEY, buildId, workspaceName); } @VisibleForTesting @Nullable public WorkspaceStatusAction getLastWorkspaceStatusAction() throws InterruptedException { WorkspaceStatusValue workspaceStatusValue = - (WorkspaceStatusValue) memoizingEvaluator.getExistingValue(WorkspaceStatusValue.SKY_KEY); + (WorkspaceStatusValue) + memoizingEvaluator.getExistingValue(WorkspaceStatusValue.BUILD_INFO_KEY); return workspaceStatusValue == null ? null : (WorkspaceStatusAction) workspaceStatusValue.getAction(0); @@ -818,11 +819,14 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { public Collection<Artifact> getWorkspaceStatusArtifacts(ExtendedEventHandler eventHandler) throws InterruptedException { // Should already be present, unless the user didn't request any targets for analysis. - EvaluationResult<WorkspaceStatusValue> result = buildDriver.evaluate( - ImmutableList.of(WorkspaceStatusValue.SKY_KEY), /*keepGoing=*/true, /*numThreads=*/1, - eventHandler); + EvaluationResult<WorkspaceStatusValue> result = + buildDriver.evaluate( + ImmutableList.of(WorkspaceStatusValue.BUILD_INFO_KEY), + /*keepGoing=*/ true, + /*numThreads=*/ 1, + eventHandler); WorkspaceStatusValue value = - Preconditions.checkNotNull(result.get(WorkspaceStatusValue.SKY_KEY)); + Preconditions.checkNotNull(result.get(WorkspaceStatusValue.BUILD_INFO_KEY)); return ImmutableList.of(value.getStableArtifact(), value.getVolatileArtifact()); } @@ -1313,8 +1317,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { for (BuildConfiguration depConfig : configs.get(key)) { skyKeys.add(ConfiguredTargetValue.key(key.getLabel(), depConfig)); for (AspectDescriptor aspectDescriptor : key.getAspects().getAllAspects()) { - skyKeys.add(ActionLookupValue.key(AspectValue.createAspectKey(key.getLabel(), depConfig, - aspectDescriptor, depConfig))); + skyKeys.add( + AspectValue.createAspectKey(key.getLabel(), depConfig, aspectDescriptor, depConfig)); } } } @@ -1346,8 +1350,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { List<ConfiguredAspect> configuredAspects = new ArrayList<>(); for (AspectDescriptor aspectDescriptor : key.getAspects().getAllAspects()) { - SkyKey aspectKey = ActionLookupValue.key(AspectValue.createAspectKey(key.getLabel(), - depConfig, aspectDescriptor, depConfig)); + SkyKey aspectKey = + AspectValue.createAspectKey(key.getLabel(), depConfig, aspectDescriptor, depConfig); if (result.get(aspectKey) == null) { continue DependentNodeLoop; } @@ -1642,7 +1646,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { List<SkyKey> keys = new ArrayList<>(ConfiguredTargetValue.keys(values)); for (AspectValueKey aspectKey : aspectKeys) { - keys.add(aspectKey.getSkyKey()); + keys.add(aspectKey); } EvaluationResult<ActionLookupValue> result = buildDriver.evaluate(keys, keepGoing, numThreads, eventHandler); @@ -1746,8 +1750,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { ArtifactOwner artifactOwner = artifact.getArtifactOwner(); Preconditions.checkState(artifactOwner instanceof ActionLookupValue.ActionLookupKey, "%s %s", artifact, artifactOwner); - SkyKey actionLookupKey = - ActionLookupValue.key((ActionLookupValue.ActionLookupKey) artifactOwner); + SkyKey actionLookupKey = (ActionLookupValue.ActionLookupKey) artifactOwner; synchronized (valueLookupLock) { // Note that this will crash (attempting to run a configured target value builder after diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java index 32e5ec5f0b..4a9946a1bc 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java @@ -32,13 +32,13 @@ public final class TestCompletionFunction implements SkyFunction { public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { TestCompletionValue.TestCompletionKey key = (TestCompletionValue.TestCompletionKey) skyKey.argument(); - ConfiguredTargetKey lac = key.configuredTargetKey(); + ConfiguredTargetKey ctKey = key.configuredTargetKey(); TopLevelArtifactContext ctx = key.topLevelArtifactContext(); - if (env.getValue(TargetCompletionValue.key(lac, ctx, /*willTest=*/true)) == null) { + if (env.getValue(TargetCompletionValue.key(ctKey, ctx, /*willTest=*/ true)) == null) { return null; } - ConfiguredTargetValue ctValue = (ConfiguredTargetValue) env.getValue(lac.getSkyKey()); + ConfiguredTargetValue ctValue = (ConfiguredTargetValue) env.getValue(ctKey); if (ctValue == null) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainUtil.java index a47e373906..11e44fca77 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainUtil.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainUtil.java @@ -144,11 +144,11 @@ public class ToolchainUtil { SkyKey executionPlatformKey = LegacySkyKey.create( SkyFunctions.CONFIGURED_TARGET, - new ConfiguredTargetKey(executionPlatformLabel, configuration)); + ConfiguredTargetKey.of(executionPlatformLabel, configuration)); SkyKey targetPlatformKey = LegacySkyKey.create( SkyFunctions.CONFIGURED_TARGET, - new ConfiguredTargetKey(targetPlatformLabel, configuration)); + ConfiguredTargetKey.of(targetPlatformLabel, configuration)); Map<SkyKey, ValueOrException<ConfiguredValueCreationException>> values = env.getValuesOrThrow( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java index e9ee29ed7c..82de66d23c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java @@ -17,7 +17,6 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; -import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.AspectDescriptor; @@ -84,11 +83,11 @@ public class ToplevelSkylarkAspectFunction implements SkyFunction { throw new LoadSkylarkAspectFunctionException(e); } SkyKey aspectKey = - ActionLookupValue.key(AspectValue.createAspectKey( - aspectLoadingKey.getTargetLabel(), aspectLoadingKey.getTargetConfiguration(), + AspectValue.createAspectKey( + aspectLoadingKey.getTargetLabel(), + aspectLoadingKey.getTargetConfiguration(), new AspectDescriptor(skylarkAspect.getAspectClass(), AspectParameters.EMPTY), - aspectLoadingKey.getAspectConfiguration() - )); + aspectLoadingKey.getAspectConfiguration()); return env.getValue(aspectKey); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java index 817c2fab54..b231bae76a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java @@ -43,7 +43,7 @@ public class WorkspaceStatusFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { Preconditions.checkState( - WorkspaceStatusValue.SKY_KEY.equals(skyKey), WorkspaceStatusValue.SKY_KEY); + WorkspaceStatusValue.BUILD_INFO_KEY.equals(skyKey), WorkspaceStatusValue.BUILD_INFO_KEY); WorkspaceNameValue workspaceNameValue = (WorkspaceNameValue) env.getValue(WorkspaceNameValue.key()); if (env.valuesMissing()) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java index 78ff03d0f3..1b695928a4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java @@ -16,11 +16,8 @@ package com.google.devtools.build.lib.skyframe; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.analysis.WorkspaceStatusAction; -import com.google.devtools.build.skyframe.LegacySkyKey; import com.google.devtools.build.skyframe.SkyFunctionName; -import com.google.devtools.build.skyframe.SkyKey; /** * Value that stores the workspace status artifacts and their generating action. There should be @@ -33,8 +30,7 @@ public class WorkspaceStatusValue extends ActionLookupValue { private final Artifact volatileArtifact; // There should only ever be one BuildInfo value in the graph. - static final ArtifactOwner ARTIFACT_OWNER = new BuildInfoKey(); - public static final SkyKey SKY_KEY = LegacySkyKey.create(SkyFunctions.BUILD_INFO, ARTIFACT_OWNER); + public static final BuildInfoKey BUILD_INFO_KEY = new BuildInfoKey(); WorkspaceStatusValue( ActionKeyContext actionKeyContext, @@ -55,15 +51,13 @@ public class WorkspaceStatusValue extends ActionLookupValue { return volatileArtifact; } - private static class BuildInfoKey extends ActionLookupKey { - @Override - protected SkyFunctionName getType() { - throw new UnsupportedOperationException(); - } + static class BuildInfoKey extends ActionLookupKey { + private BuildInfoKey() {} @Override - protected SkyKey getSkyKeyInternal() { - return SKY_KEY; + public SkyFunctionName functionName() { + return SkyFunctions.BUILD_INFO; } + } } |