Only create the host configuration once per BuildConfiguration instance.
We seem to be calling this function many times creating a lot of garbage
(when creating the FragmentClassSet to be used as a cache key). However,
in regular builds we are currently only calling this with a few different
instances of BuildConfiguration ever.
RELNOTES: None.
PiperOrigin-RevId: 241897160
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 6ff9493..a090655 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
@@ -134,7 +134,7 @@
private BuildConfiguration topLevelHostConfiguration;
// Fragment-limited versions of the host configuration. It's faster to create/cache these here
// than to store them in Skyframe.
- private Map<FragmentClassSet, BuildConfiguration> hostConfigurationCache =
+ private Map<BuildConfiguration, BuildConfiguration> hostConfigurationCache =
Maps.newConcurrentMap();
private BuildConfigurationCollection configurations;
@@ -787,6 +787,16 @@
if (config == null) {
return topLevelHostConfiguration;
}
+ // Currently, a single build doesn't use many different BuildConfiguration instances. Thus,
+ // having a cache per BuildConfiguration is efficient. It might lead to instances of otherwise
+ // identical configurations if multiple of these configs use the same fragment classes. However,
+ // these are cheap especially if there is only a small number of configs. Revisit and turn into
+ // a cache per FragmentClassSet if configuration trimming results in a much higher number of
+ // configuration instances.
+ BuildConfiguration hostConfig = hostConfigurationCache.get(config);
+ if (hostConfig != null) {
+ return hostConfig;
+ }
// TODO(bazel-team): have the fragment classes be those required by the consuming target's
// transitive closure. This isn't the same as the input configuration's fragment classes -
// the latter may be a proper subset of the former.
@@ -805,10 +815,6 @@
config.trimConfigurations()
? config.fragmentClasses()
: FragmentClassSet.of(ruleClassProvider.getAllFragments());
- BuildConfiguration hostConfig = hostConfigurationCache.get(fragmentClasses);
- if (hostConfig != null) {
- return hostConfig;
- }
// TODO(bazel-team): investigate getting the trimmed config from Skyframe instead of cloning.
// This is the only place we instantiate BuildConfigurations outside of Skyframe, This can
// produce surprising effects, such as requesting a configuration that's in the Skyframe cache
@@ -822,7 +828,7 @@
BuildConfiguration trimmedConfig =
topLevelHostConfiguration.clone(
fragmentClasses, ruleClassProvider, skyframeExecutor.getDefaultBuildOptions());
- hostConfigurationCache.put(fragmentClasses, trimmedConfig);
+ hostConfigurationCache.put(config, trimmedConfig);
return trimmedConfig;
}
