Skip to content

Commit

Permalink
Break out a StarlarkTransitionCache from ConfigurationResolver an…
Browse files Browse the repository at this point in the history 
…d 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
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Aug 8, 2022
1 parent b44b9e4 commit 6efc2ab
Show file tree
Hide file tree
Showing 9 changed files with 252 additions and 193 deletions.
15 changes: 15 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
179 changes: 19 additions & 160 deletions src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -62,7 +57,6 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -95,95 +89,19 @@ public final class ConfigurationResolver {
private final TargetAndConfiguration ctgValue;
private final BuildConfigurationValue hostConfiguration;
private final ImmutableMap<Label, ConfigMatchingProvider> 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<String, BuildOptions> 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.
*
* <p>This is null if the transition lacks non-error events.
*/
@Nullable final StoredEventHandler nonErrorEvents;

StarlarkTransitionCacheValue(
Map<String, BuildOptions> result, @Nullable StoredEventHandler nonErrorEvents) {
this.result = result;
this.nonErrorEvents = nonErrorEvents;
}
}

/**
* Caches the application of transitions that use Starlark.
*
* <p>This trivially includes {@link StarlarkTransition}s. But it also includes transitions that
* delegate to {@link StarlarkTransition}s, like some {@link ComposingTransition}s.
*
* <p>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<StarlarkTransitionCacheKey, StarlarkTransitionCacheValue>
starlarkTransitionCache = Caffeine.newBuilder().softValues().build();
private final StarlarkTransitionCache starlarkTransitionCache;

public ConfigurationResolver(
SkyFunction.Environment env,
TargetAndConfiguration ctgValue,
BuildConfigurationValue hostConfiguration,
ImmutableMap<Label, ConfigMatchingProvider> configConditions) {
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
StarlarkTransitionCache starlarkTransitionCache) {
this.env = env;
this.ctgValue = ctgValue;
this.hostConfiguration = hostConfiguration;
this.configConditions = configConditions;
this.starlarkTransitionCache = starlarkTransitionCache;
}

private BuildConfigurationValue getCurrentConfiguration() {
Expand Down Expand Up @@ -312,12 +230,7 @@ private ImmutableList<Dependency> resolveGenericTransition(
throws ConfiguredValueCreationException, InterruptedException {
Map<String, BuildOptions> 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.
}
Expand Down Expand Up @@ -409,9 +322,7 @@ private ImmutableList<String> collectTransitionKeys(
ConfigurationTransition baseTransition = transitionFactory.create(transitionData);
Map<String, BuildOptions> 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.
}
Expand Down Expand Up @@ -445,10 +356,12 @@ public static Map<String, BuildOptions> 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);
}
Expand All @@ -467,24 +380,26 @@ public static Map<String, BuildOptions> applyTransitionWithoutSkyframe(
* respective packages.
*/
@Nullable
public static Map<String, BuildOptions> applyTransitionWithSkyframe(
BuildOptions fromOptions,
ConfigurationTransition transition,
SkyFunction.Environment env,
ExtendedEventHandler eventHandler)
private Map<String, BuildOptions> 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)
Expand All @@ -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<String, BuildOptions> 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<String, BuildOptions> 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.
*
Expand Down
Loading

0 comments on commit 6efc2ab

Please sign in to comment.