From 6efc2ab2302f31dd522bcf955bf23cec4f1a95b5 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 8 Aug 2022 14:57:19 -0700 Subject: [PATCH] Break out a `StarlarkTransitionCache` from `ConfigurationResolver` and store it in `SkyframeBuildView`. This replaces the static cache so that soft references don't stick around after a `clean` or non-incremental build. See the description of unknown commit, which makes a similar change. Additionally, we can clear the `StarlarkTransitionCache` when the analysis cache is discarded. PiperOrigin-RevId: 466157315 Change-Id: I2501ade99e9614560bc7d1db1b593e32b8ad2835 --- .../google/devtools/build/lib/analysis/BUILD | 15 ++ .../config/ConfigurationResolver.java | 179 ++---------------- .../config/StarlarkTransitionCache.java | 164 ++++++++++++++++ .../build/lib/skyframe/AspectFunction.java | 11 +- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../skyframe/ConfiguredTargetFunction.java | 25 ++- .../build/lib/skyframe/SkyframeBuildView.java | 15 +- .../build/lib/skyframe/SkyframeExecutor.java | 20 +- .../ConfigurationsForTargetsTest.java | 15 +- 9 files changed, 252 insertions(+), 193 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkTransitionCache.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index d486b81c9f9760..7410f484c6b1bf 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -298,6 +298,7 @@ java_library( ":config/per_label_options", ":config/run_under", ":config/starlark_defined_config_transition", + ":config/starlark_transition_cache", ":config/toolchain_type_requirement", ":config/transition_factories", ":config/transitions/composing_transition", @@ -1939,6 +1940,20 @@ java_library( ], ) +java_library( + name = "config/starlark_transition_cache", + srcs = ["config/StarlarkTransitionCache.java"], + deps = [ + ":config/build_options", + ":config/transitions/configuration_transition", + ":starlark/starlark_build_settings_details_value", + ":starlark/starlark_transition", + "//src/main/java/com/google/devtools/build/lib/events", + "//third_party:caffeine", + "//third_party:jsr305", + ], +) + # TODO(b/144899336): This should be config/transitions/BUILD java_library( name = "config/transitions/composing_transition", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java index ed4125a950e9ff..4ff761839cbba6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java @@ -14,8 +14,6 @@ package com.google.devtools.build.lib.analysis.config; -import com.github.benmanes.caffeine.cache.Cache; -import com.github.benmanes.caffeine.cache.Caffeine; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Functions; import com.google.common.collect.ImmutableList; @@ -29,19 +27,16 @@ import com.google.devtools.build.lib.analysis.DependencyKind; import com.google.devtools.build.lib.analysis.PlatformOptions; import com.google.devtools.build.lib.analysis.TargetAndConfiguration; -import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransition; import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; import com.google.devtools.build.lib.analysis.config.transitions.NullTransition; import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition; import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory; import com.google.devtools.build.lib.analysis.config.transitions.TransitionUtil; -import com.google.devtools.build.lib.analysis.starlark.FunctionTransitionUtil; import com.google.devtools.build.lib.analysis.starlark.StarlarkBuildSettingsDetailsValue; import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition; import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition.TransitionException; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.ExtendedEventHandler; -import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.AttributeTransitionData; import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper; @@ -62,7 +57,6 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.Objects; import javax.annotation.Nullable; /** @@ -95,95 +89,19 @@ public final class ConfigurationResolver { private final TargetAndConfiguration ctgValue; private final BuildConfigurationValue hostConfiguration; private final ImmutableMap configConditions; - - /** The key for {@link #starlarkTransitionCache}. */ - private static class StarlarkTransitionCacheKey { - private final ConfigurationTransition transition; - private final BuildOptions fromOptions; - private final StarlarkBuildSettingsDetailsValue details; - private final int hashCode; - - StarlarkTransitionCacheKey( - ConfigurationTransition transition, - BuildOptions fromOptions, - StarlarkBuildSettingsDetailsValue details) { - // For rule self-transitions, the transition instance encapsulates both the transition logic - // and attributes of the target it's attached to. This is important: the same transition in - // the same configuration applied to distinct targets may produce different outputs. See - // StarlarkRuleTransitionProvider.FunctionPatchTransition for details. - // TODO(bazel-team): the transition code (i.e. StarlarkTransitionFunction) hashes on identity. - // Check that unnecessary copies of the transition function don't dilute this cache. Quick - // experimentation shows the # of such instances is very small. But it's unclear how strong - // of an interning contract there is. - this.transition = transition; - this.fromOptions = fromOptions; - this.details = details; - this.hashCode = Objects.hash(transition, fromOptions, details); - } - - @Override - public boolean equals(Object other) { - if (other == this) { - return true; - } - if (!(other instanceof StarlarkTransitionCacheKey)) { - return false; - } - return (this.transition.equals(((StarlarkTransitionCacheKey) other).transition) - && this.fromOptions.equals(((StarlarkTransitionCacheKey) other).fromOptions) - && this.details.equals(((StarlarkTransitionCacheKey) other).details)); - } - - @Override - public int hashCode() { - return hashCode; - } - } - - /** The result of a {@link #starlarkTransitionCache} lookup. */ - private static class StarlarkTransitionCacheValue { - final Map result; - /** - * Stores events for successful transitions. Transitions that fail aren't added to the cache. - * This is meant for non-error events like Starlark {@code print()} output. See {@link - * StarlarkIntegrationTest#testPrintFromTransitionImpl} for a test that covers this. - * - *

This is null if the transition lacks non-error events. - */ - @Nullable final StoredEventHandler nonErrorEvents; - - StarlarkTransitionCacheValue( - Map result, @Nullable StoredEventHandler nonErrorEvents) { - this.result = result; - this.nonErrorEvents = nonErrorEvents; - } - } - - /** - * Caches the application of transitions that use Starlark. - * - *

This trivially includes {@link StarlarkTransition}s. But it also includes transitions that - * delegate to {@link StarlarkTransition}s, like some {@link ComposingTransition}s. - * - *

This cache was added to keep builds that heavily rely on Starlark transitions performant. - * The inspiring build is a large Apple binary that heavily relies on {@code objc_library.bzl}, - * which applies a self-transition. The build applies this transition ~600,000 times. Each - * application has a cost, mostly from setup in translating Java objects to Starlark objects in - * {@link FunctionTransitionUtil#applyAndValidate}. This cache saves most of that work, reducing - * analysis phase CPU time by 17%. - */ - private static final Cache - starlarkTransitionCache = Caffeine.newBuilder().softValues().build(); + private final StarlarkTransitionCache starlarkTransitionCache; public ConfigurationResolver( SkyFunction.Environment env, TargetAndConfiguration ctgValue, BuildConfigurationValue hostConfiguration, - ImmutableMap configConditions) { + ImmutableMap configConditions, + StarlarkTransitionCache starlarkTransitionCache) { this.env = env; this.ctgValue = ctgValue; this.hostConfiguration = hostConfiguration; this.configConditions = configConditions; + this.starlarkTransitionCache = starlarkTransitionCache; } private BuildConfigurationValue getCurrentConfiguration() { @@ -312,12 +230,7 @@ private ImmutableList resolveGenericTransition( throws ConfiguredValueCreationException, InterruptedException { Map toOptions; try { - toOptions = - applyTransitionWithSkyframe( - getCurrentConfiguration().getOptions(), - dependencyKey.getTransition(), - env, - eventHandler); + toOptions = applyTransitionWithSkyframe(dependencyKey.getTransition(), eventHandler); if (toOptions == null) { return null; // Need more Skyframe deps for a Starlark transition. } @@ -409,9 +322,7 @@ private ImmutableList collectTransitionKeys( ConfigurationTransition baseTransition = transitionFactory.create(transitionData); Map toOptions; try { - toOptions = - applyTransitionWithSkyframe( - getCurrentConfiguration().getOptions(), baseTransition, env, eventHandler); + toOptions = applyTransitionWithSkyframe(baseTransition, eventHandler); if (toOptions == null) { return null; // Need more Skyframe deps for a Starlark transition. } @@ -445,10 +356,12 @@ public static Map applyTransitionWithoutSkyframe( BuildOptions fromOptions, ConfigurationTransition transition, StarlarkBuildSettingsDetailsValue details, - ExtendedEventHandler eventHandler) + ExtendedEventHandler eventHandler, + StarlarkTransitionCache starlarkTransitionCache) throws TransitionException, InterruptedException { if (StarlarkTransition.doesStarlarkTransition(transition)) { - return applyStarlarkTransition(fromOptions, transition, details, eventHandler); + return starlarkTransitionCache.computeIfAbsent( + fromOptions, transition, details, eventHandler); } return transition.apply(TransitionUtil.restrict(transition, fromOptions), eventHandler); } @@ -467,24 +380,26 @@ public static Map applyTransitionWithoutSkyframe( * respective packages. */ @Nullable - public static Map applyTransitionWithSkyframe( - BuildOptions fromOptions, - ConfigurationTransition transition, - SkyFunction.Environment env, - ExtendedEventHandler eventHandler) + private Map applyTransitionWithSkyframe( + ConfigurationTransition transition, ExtendedEventHandler eventHandler) throws TransitionException, InterruptedException { + BuildOptions fromOptions = getCurrentConfiguration().getOptions(); if (StarlarkTransition.doesStarlarkTransition(transition)) { StarlarkBuildSettingsDetailsValue details = getStarlarkBuildSettingsDetailsValue(transition, env); if (details == null) { return null; } - return applyStarlarkTransition(fromOptions, transition, details, eventHandler); + return starlarkTransitionCache.computeIfAbsent( + fromOptions, transition, details, eventHandler); } return transition.apply(TransitionUtil.restrict(transition, fromOptions), eventHandler); } - /** Must be in sync with {@link SkyframeExecutor.getStarlarkBuildSettingsDetailsValue} */ + /** + * Must be in sync with {@link + * com.google.devtools.build.lib.skyframe.SkyframeExecutor#getStarlarkBuildSettingsDetailsValue} + */ @Nullable private static StarlarkBuildSettingsDetailsValue getStarlarkBuildSettingsDetailsValue( ConfigurationTransition transition, SkyFunction.Environment env) @@ -502,62 +417,6 @@ private static StarlarkBuildSettingsDetailsValue getStarlarkBuildSettingsDetails TransitionException.class); } - /** - * Applies a Starlark transition. - * - * @param fromOptions source options before the transition - * @param transition the transition itself - * @param details information from packages about Starlark build settings needed by transition - * @param eventHandler handler for errors evaluating the transition. - * @return transition output - */ - private static Map applyStarlarkTransition( - BuildOptions fromOptions, - ConfigurationTransition transition, - StarlarkBuildSettingsDetailsValue details, - ExtendedEventHandler eventHandler) - throws TransitionException, InterruptedException { - StarlarkTransitionCacheKey cacheKey = - new StarlarkTransitionCacheKey(transition, fromOptions, details); - StarlarkTransitionCacheValue cachedResult = starlarkTransitionCache.getIfPresent(cacheKey); - if (cachedResult != null) { - if (cachedResult.nonErrorEvents != null) { - cachedResult.nonErrorEvents.replayOn(eventHandler); - } - return cachedResult.result; - } - // All code below here only executes on a cache miss and thus should rely only on values that - // are part of the above cache key or constants that exist throughout the lifetime of the - // Blaze server instance. - BuildOptions adjustedOptions = - StarlarkTransition.addDefaultStarlarkOptions(fromOptions, transition, details); - // TODO(bazel-team): Add safety-check that this never mutates fromOptions. - StoredEventHandler handlerWithErrorStatus = new StoredEventHandler(); - Map result = - transition.apply( - TransitionUtil.restrict(transition, adjustedOptions), handlerWithErrorStatus); - - // We use a temporary StoredEventHandler instead of the caller's event handler because - // StarlarkTransition.validate assumes no errors occurred. We need a StoredEventHandler to be - // able to check that, and fail out early if there are errors. - // - // TODO(bazel-team): harden StarlarkTransition.validate so we can eliminate this step. - // StarlarkRuleTransitionProviderTest#testAliasedBuildSetting_outputReturnMismatch shows the - // effect. - handlerWithErrorStatus.replayOn(eventHandler); - if (handlerWithErrorStatus.hasErrors()) { - throw new TransitionException("Errors encountered while applying Starlark transition"); - } - result = StarlarkTransition.validate(transition, details, result); - // If the transition errored (like bad Starlark code), this method already exited with an - // exception so the results won't go into the cache. We still want to collect non-error events - // like print() output. - StoredEventHandler nonErrorEvents = - !handlerWithErrorStatus.isEmpty() ? handlerWithErrorStatus : null; - starlarkTransitionCache.put(cacheKey, new StarlarkTransitionCacheValue(result, nonErrorEvents)); - return result; - } - /** * This method allows resolution of configurations outside of a skyfunction call. * diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkTransitionCache.java b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkTransitionCache.java new file mode 100644 index 00000000000000..70ba79d77bcd90 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkTransitionCache.java @@ -0,0 +1,164 @@ +// Copyright 2017 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.analysis.config; + +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; +import com.google.devtools.build.lib.analysis.config.transitions.TransitionUtil; +import com.google.devtools.build.lib.analysis.starlark.StarlarkBuildSettingsDetailsValue; +import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition; +import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition.TransitionException; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.events.StoredEventHandler; +import java.util.Map; +import java.util.Objects; +import javax.annotation.Nullable; + +/** + * Caches the application of transitions that use Starlark. + * + *

This trivially includes {@link StarlarkTransition}s. But it also includes transitions that + * delegate to {@link StarlarkTransition}s, like some {@link + * com.google.devtools.build.lib.analysis.config.transitions.ComposingTransition}s. + * + *

This cache was added to keep builds that heavily rely on Starlark transitions performant. The + * inspiring build is a large Apple binary that heavily relies on {@code objc_library.bzl}, which + * applies a self-transition. The build applies this transition ~600,000 times. Each application has + * a cost, mostly from setup in translating Java objects to Starlark objects in {@link + * com.google.devtools.build.lib.analysis.starlark.FunctionTransitionUtil#applyAndValidate}. This + * cache saves most of that work, reducing analysis phase CPU time by 17%. + */ +public final class StarlarkTransitionCache { + + private Cache cache = Caffeine.newBuilder().softValues().build(); + + /** + * Applies a Starlark transition, possibly returning a cached result. + * + * @param fromOptions source options before the transition + * @param transition the transition itself + * @param details information from packages about Starlark build settings needed by transition + * @param eventHandler handler for errors evaluating the transition. + * @return transition output + */ + Map computeIfAbsent( + BuildOptions fromOptions, + ConfigurationTransition transition, + StarlarkBuildSettingsDetailsValue details, + ExtendedEventHandler eventHandler) + throws TransitionException, InterruptedException { + Key cacheKey = new Key(transition, fromOptions, details); + Value cachedResult = cache.getIfPresent(cacheKey); + if (cachedResult != null) { + if (cachedResult.nonErrorEvents != null) { + cachedResult.nonErrorEvents.replayOn(eventHandler); + } + return cachedResult.result; + } + // All code below here only executes on a cache miss and thus should rely only on values that + // are part of the above cache key or constants that exist throughout the lifetime of the + // Blaze server instance. + BuildOptions adjustedOptions = + StarlarkTransition.addDefaultStarlarkOptions(fromOptions, transition, details); + // TODO(bazel-team): Add safety-check that this never mutates fromOptions. + StoredEventHandler handlerWithErrorStatus = new StoredEventHandler(); + Map result = + transition.apply( + TransitionUtil.restrict(transition, adjustedOptions), handlerWithErrorStatus); + + // We use a temporary StoredEventHandler instead of the caller's event handler because + // StarlarkTransition.validate assumes no errors occurred. We need a StoredEventHandler to be + // able to check that, and fail out early if there are errors. + // + // TODO(bazel-team): harden StarlarkTransition.validate so we can eliminate this step. + // StarlarkRuleTransitionProviderTest#testAliasedBuildSetting_outputReturnMismatch shows the + // effect. + handlerWithErrorStatus.replayOn(eventHandler); + if (handlerWithErrorStatus.hasErrors()) { + throw new TransitionException("Errors encountered while applying Starlark transition"); + } + result = StarlarkTransition.validate(transition, details, result); + // If the transition errored (like bad Starlark code), this method already exited with an + // exception so the results won't go into the cache. We still want to collect non-error events + // like print() output. + StoredEventHandler nonErrorEvents = + !handlerWithErrorStatus.isEmpty() ? handlerWithErrorStatus : null; + cache.put(cacheKey, new Value(result, nonErrorEvents)); + return result; + } + + public void clear() { + cache = Caffeine.newBuilder().softValues().build(); + } + + private static final class Key { + private final ConfigurationTransition transition; + private final BuildOptions fromOptions; + private final StarlarkBuildSettingsDetailsValue details; + private final int hashCode; + + private Key( + ConfigurationTransition transition, + BuildOptions fromOptions, + StarlarkBuildSettingsDetailsValue details) { + // For rule self-transitions, the transition instance encapsulates both the transition logic + // and attributes of the target it's attached to. This is important: the same transition in + // the same configuration applied to distinct targets may produce different outputs. See + // StarlarkRuleTransitionProvider.FunctionPatchTransition for details. + this.transition = transition; + this.fromOptions = fromOptions; + this.details = details; + this.hashCode = Objects.hash(transition, fromOptions, details); + } + + @Override + public boolean equals(Object other) { + if (other == this) { + return true; + } + if (!(other instanceof Key)) { + return false; + } + Key otherKey = (Key) other; + return this.transition.equals(otherKey.transition) + && this.fromOptions.equals(otherKey.fromOptions) + && this.details.equals(otherKey.details); + } + + @Override + public int hashCode() { + return hashCode; + } + } + + private static final class Value { + private final Map result; + /** + * Stores events for successful transitions. Transitions that fail aren't added to the cache. + * This is meant for non-error events like Starlark {@code print()} output. See {@link + * com.google.devtools.build.lib.starlark.StarlarkIntegrationTest#testPrintFromTransitionImpl} + * for a test that covers this. + * + *

This is null if the transition lacks non-error events. + */ + @Nullable private final StoredEventHandler nonErrorEvents; + + Value(Map result, @Nullable StoredEventHandler nonErrorEvents) { + this.result = result; + this.nonErrorEvents = nonErrorEvents; + } + } +} 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 45eb7affc51ade..46b49b4035b462 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 @@ -207,7 +207,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) if (AliasProvider.isAlias(associatedTarget)) { return createAliasAspect( env, - buildViewProvider.getSkyframeBuildView().getHostConfiguration(), new TargetAndConfiguration(target, configuration), aspect, key, @@ -333,7 +332,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) ? null : unloadedToolchainContexts.asToolchainContexts(), ruleClassProvider, - buildViewProvider.getSkyframeBuildView().getHostConfiguration()); + buildViewProvider.getSkyframeBuildView()); } catch (ConfiguredValueCreationException e) { throw new AspectCreationException( e.getMessage(), key.getLabel(), configuration, e.getDetailedExitCode()); @@ -432,7 +431,7 @@ static BzlLoadValue.Key bzlLoadKeyForStarlarkAspect(StarlarkAspectClass starlark } @Nullable - private InitialValues getInitialValues(AspectKey key, Environment env) + private static InitialValues getInitialValues(AspectKey key, Environment env) throws AspectFunctionException, InterruptedException { StarlarkAspectClass starlarkAspectClass; ConfiguredAspectFactory aspectFactory = null; @@ -623,7 +622,6 @@ private static void collectAspectKeysInTopologicalOrder( @Nullable private AspectValue createAliasAspect( Environment env, - BuildConfigurationValue hostConfiguration, TargetAndConfiguration originalTarget, Aspect aspect, AspectKey originalKey, @@ -694,8 +692,9 @@ private AspectValue createAliasAspect( new ConfigurationResolver( env, originalTargetAndAspectConfiguration, - hostConfiguration, - configConditions.asProviders()); + buildViewProvider.getSkyframeBuildView().getHostConfiguration(), + configConditions.asProviders(), + buildViewProvider.getSkyframeBuildView().getStarlarkTransitionCache()); ImmutableList deps = resolver.resolveConfiguration(depKind, depKey, env.getListener()); if (deps == null) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index f324517c09acdb..e7bca0485d851c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -246,6 +246,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception", "//src/main/java/com/google/devtools/build/lib/analysis:config/optioninfo", "//src/main/java/com/google/devtools/build/lib/analysis:config/starlark_defined_config_transition", + "//src/main/java/com/google/devtools/build/lib/analysis:config/starlark_transition_cache", "//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition", 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 ddcc8d29fbc390..0e0151589893ff 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 @@ -281,7 +281,7 @@ public SkyValue compute(SkyKey key, Environment env) SkyframeBuildView view = buildViewProvider.getSkyframeBuildView(); SkyframeDependencyResolver resolver = new SkyframeDependencyResolver(env); ToolchainCollection unloadedToolchainContexts = null; - ExecGroupCollection.Builder execGroupCollectionBuilder = null; + ExecGroupCollection.Builder execGroupCollectionBuilder; // TODO(janakr): this call may tie up this thread indefinitely, reducing the parallelism of // Skyframe. This is a strict improvement over the prior state of the code, in which we ran @@ -350,7 +350,7 @@ public SkyValue compute(SkyKey key, Environment env) ? null : unloadedToolchainContexts.asToolchainContexts(), ruleClassProvider, - view.getHostConfiguration()); + view); if (!state.transitiveRootCauses.isEmpty()) { NestedSet causes = state.transitiveRootCauses.build(); // TODO(bazel-team): consider reporting the error in this class vs. exporting it for @@ -464,7 +464,7 @@ public SkyValue compute(SkyKey key, Environment env) } @Nullable - private TargetAndConfiguration getTargetAndConfiguration( + private static TargetAndConfiguration getTargetAndConfiguration( ConfiguredTargetKey configuredTargetKey, State state, Environment env) throws InterruptedException, ReportedException { if (state.targetAndConfiguration != null) { @@ -745,9 +745,7 @@ public static ImmutableSet

Throw a ViewCreationFailedException in case of --nokeep_going. */ - private void reportActionConflictErrors( + private static void reportActionConflictErrors( TopLevelActionConflictReport topLevelActionConflictReport, Iterable effectiveTopLevelKeysForConflictReporting, ExtendedEventHandler eventHandler, @@ -1000,7 +1004,7 @@ private static ImmutableSet getSuccessfulConfiguredTargets( return cts.build(); } - private ImmutableMap getSuccessfulAspectMap( + private static ImmutableMap getSuccessfulAspectMap( int expectedSize, EvaluationResult evaluationResult, Set buildDriverAspectKeys, @@ -1220,6 +1224,7 @@ public BuildConfigurationValue getHostConfiguration() { */ void clearLegacyData() { artifactFactory.clear(); + starlarkTransitionCache.clear(); } /** @@ -1259,6 +1264,10 @@ public ActionKeyContext getActionKeyContext() { return skyframeExecutor.getActionKeyContext(); } + public StarlarkTransitionCache getStarlarkTransitionCache() { + return starlarkTransitionCache; + } + private final class ActionLookupValueProgressReceiver implements EvaluationProgressReceiver { private final AtomicInteger configuredObjectCount = new AtomicInteger(); private final AtomicInteger actionCount = new AtomicInteger(); 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 32284013173569..bfa99503052769 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 @@ -1109,7 +1109,7 @@ protected boolean processDiscardAndDetermineRemoval( } /** Tracks how long it takes to clear the analysis cache. */ - private final SilentCloseable trackDiscardAnalysisCache() { + private SilentCloseable trackDiscardAnalysisCache() { AutoProfiler profiler = GoogleAutoProfilerUtils.logged("discarding analysis cache"); return () -> { Duration d = Duration.ofNanos(profiler.completeAndGetElapsedTimeNanos()); @@ -1919,7 +1919,11 @@ public ConfigurationsResult getConfigurations( getStarlarkBuildSettingsDetailsValue(eventHandler, transition); toOptions = ConfigurationResolver.applyTransitionWithoutSkyframe( - fromOptions, transition, details, eventHandler) + fromOptions, + transition, + details, + eventHandler, + skyframeBuildView.getStarlarkTransitionCache()) .values(); } catch (TransitionException e) { eventHandler.handle(Event.error(e.getMessage())); @@ -1945,7 +1949,11 @@ public ConfigurationsResult getConfigurations( getStarlarkBuildSettingsDetailsValue(eventHandler, key.getTransition()); toOptions = ConfigurationResolver.applyTransitionWithoutSkyframe( - fromOptions, key.getTransition(), details, eventHandler) + fromOptions, + key.getTransition(), + details, + eventHandler, + skyframeBuildView.getStarlarkTransitionCache()) .values(); } catch (TransitionException e) { eventHandler.handle(Event.error(e.getMessage())); @@ -1974,7 +1982,7 @@ public ConfigurationsResult getConfigurations( return builder.build(); } - /** Must be in sync with {@link ConfigurationResolver.getStarlarkBuildSettingsDetailsValue} */ + /** Must be in sync with {@link ConfigurationResolver#getStarlarkBuildSettingsDetailsValue}. */ private StarlarkBuildSettingsDetailsValue getStarlarkBuildSettingsDetailsValue( ExtendedEventHandler eventHandler, ConfigurationTransition transition) throws TransitionException { @@ -1996,7 +2004,7 @@ private StarlarkBuildSettingsDetailsValue getStarlarkBuildSettingsDetailsValue( Iterables.getFirst(newlyLoaded.errorMap().entrySet(), null), newlyLoaded); throw new TransitionException( "Error when resolving transition build settings, " - + starlarkBuildSettings.toString() + + starlarkBuildSettings + ": " + errorEntry.getValue().getException()); } @@ -2939,7 +2947,7 @@ public RepositoryMapping getMainRepoMapping(ExtendedEventHandler eventHandler) } @Nullable - RuleContextConstraintSemantics getRuleContextConstraintSemantics() { + private RuleContextConstraintSemantics getRuleContextConstraintSemantics() { return ruleContextConstraintSemantics; } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java index 1ca1a5856badc4..48a821fbc3cd5b 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java @@ -49,7 +49,6 @@ import com.google.devtools.build.skyframe.EvaluationResult; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; -import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; @@ -81,11 +80,11 @@ * direct access to environments. */ @RunWith(JUnit4.class) -public class ConfigurationsForTargetsTest extends AnalysisTestCase { +public final class ConfigurationsForTargetsTest extends AnalysisTestCase { - public static final Label TARGET_PLATFORM_LABEL = + private static final Label TARGET_PLATFORM_LABEL = Label.parseAbsoluteUnchecked("//platform:target"); - public static final Label EXEC_PLATFORM_LABEL = Label.parseAbsoluteUnchecked("//platform:exec"); + private static final Label EXEC_PLATFORM_LABEL = Label.parseAbsoluteUnchecked("//platform:exec"); /** * A mock {@link SkyFunction} that just calls {@link ConfiguredTargetFunction#computeDependencies} @@ -159,7 +158,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) ImmutableMap.of(), toolchainContexts, stateProvider.lateBoundRuleClassProvider(), - stateProvider.lateBoundHostConfig()); + stateProvider.lateBoundSkyframeBuildView()); return env.valuesMissing() ? null : new Value(depMap); } catch (RuntimeException e) { throw e; @@ -191,8 +190,8 @@ RuleClassProvider lateBoundRuleClassProvider() { return ruleClassProvider; } - BuildConfigurationValue lateBoundHostConfig() { - return getHostConfiguration(); + SkyframeBuildView lateBoundSkyframeBuildView() { + return skyframeExecutor.getSkyframeBuildView(); } } @@ -244,7 +243,7 @@ private Multimap getConfiguredDeps( * *

Throws an exception if the attribute can't be found. */ - protected List getConfiguredDeps(String targetLabel, String attrName) + private ImmutableList getConfiguredDeps(String targetLabel, String attrName) throws Exception { ConfiguredTarget target = Iterables.getOnlyElement(update(targetLabel).getTargetsToBuild()); ImmutableList maybeConfiguredDeps = getConfiguredDeps(target, attrName);