Skip to content

Commit

Permalink
Move fragment cache infra from BuildConfigurationFunction to new Frag…
Browse files Browse the repository at this point in the history
…mentFactory

This is a little cleaner as now BuildConfigurationFunction is simpler and it is clear what code is part of fragment and creation (moved to FragmentFactory).

PiperOrigin-RevId: 412963297
  • Loading branch information
twigg authored and copybara-github committed Nov 29, 2021
1 parent 4e24829 commit af15543
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 76 deletions.
16 changes: 16 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1704,6 +1704,22 @@ java_library(
],
)

java_library(
name = "config/fragment_factory",
srcs = ["config/FragmentFactory.java"],
deps = [
":config/build_options",
":config/core_options",
":config/fragment",
":config/fragment_options",
":config/invalid_configuration_exception",
"//third_party:auto_value",
"//third_party:caffeine",
"//third_party:guava",
"//third_party:jsr305",
],
)

java_library(
name = "config/fragment_registry",
srcs = ["config/FragmentRegistry.java"],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
// Copyright 2021 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.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.google.auto.value.AutoValue;
import com.google.common.base.Throwables;
import java.lang.reflect.InvocationTargetException;
import java.util.Set;
import java.util.concurrent.CompletionException;
import javax.annotation.Nullable;

/** Handles construction of {@link Fragment} from a {@link BuildOptions}. */
public final class FragmentFactory {

/**
* Creates the requested {@link Fragment} using a given {@link BuildOptions}.
*
* <p>Returns null if the fragment could not be built (e.g. the supplied BuildOptions does not
* contain the required {@link FragmentOption}s).
*/
@Nullable
public Fragment createFragment(BuildOptions buildOptions, Class<? extends Fragment> fragmentClass)
throws InvalidConfigurationException {
BuildOptions trimmedOptions = trimToRequiredOptions(buildOptions, fragmentClass);
Fragment fragment;
FragmentKey fragmentKey = FragmentKey.create(trimmedOptions, fragmentClass);
try {
fragment = fragmentCache.get(fragmentKey);
} catch (CompletionException e) {
Throwables.propagateIfPossible(e.getCause(), InvalidConfigurationException.class);
throw e;
}
if (fragment != NULL_MARKER) {
return fragment;
} else {
// NULL_MARKER is never GC'ed, so this entry will stay in cache forever unless we delete it
// ourselves. Since it's a cheap computation we don't care about recomputing it.
fragmentCache.invalidate(fragmentKey);
return null;
}
}

/** Cache and associated infrastructure* */
// Cache with weak values can't have null values.
// TODO(blaze-configurability-team): At the moment, the only time shouldInclude is false is when
// TestFragment is constructed without TestOptions, which is already being registered as a
// required option of TestFragment. Should just abort fragment construction early when a
// required option is missing rather than use this NULL_MARKER infra.
private static final Fragment NULL_MARKER = new Fragment() {};

private final LoadingCache<FragmentKey, Fragment> fragmentCache =
Caffeine.newBuilder().weakValues().build(FragmentFactory::makeFragment);

private static BuildOptions trimToRequiredOptions(
BuildOptions original, Class<? extends Fragment> fragment) {
BuildOptions.Builder trimmed = BuildOptions.builder();
Set<Class<? extends FragmentOptions>> requiredOptions = Fragment.requiredOptions(fragment);
for (FragmentOptions options : original.getNativeOptions()) {
// CoreOptions is implicitly required by all fragments.
if (options instanceof CoreOptions || requiredOptions.contains(options.getClass())) {
trimmed.addFragmentOptions(options);
}
}
if (Fragment.requiresStarlarkOptions(fragment)) {
trimmed.addStarlarkOptions(original.getStarlarkOptions());
}
return trimmed.build();
}

@AutoValue
abstract static class FragmentKey {
// These BuildOptions should be already-trimmed to maximize cache efficacy
abstract BuildOptions getBuildOptions();

abstract Class<? extends Fragment> getFragmentClass();

private static FragmentKey create(
BuildOptions buildOptions, Class<? extends Fragment> fragmentClass) {
return new AutoValue_FragmentFactory_FragmentKey(buildOptions, fragmentClass);
}
}

private static Fragment makeFragment(FragmentKey fragmentKey)
throws InvalidConfigurationException {
BuildOptions buildOptions = fragmentKey.getBuildOptions();
Class<? extends Fragment> fragmentClass = fragmentKey.getFragmentClass();
String noConstructorPattern = "%s lacks constructor(BuildOptions)";
try {
Fragment fragment =
fragmentClass.getConstructor(BuildOptions.class).newInstance(buildOptions);
return fragment.shouldInclude() ? fragment : NULL_MARKER;
} catch (InvocationTargetException e) {
if (e.getCause() instanceof InvalidConfigurationException) {
throw (InvalidConfigurationException) e.getCause();
}
throw new IllegalStateException(String.format(noConstructorPattern, fragmentClass), e);
} catch (ReflectiveOperationException e) {
throw new IllegalStateException(String.format(noConstructorPattern, fragmentClass), e);
}
}
}
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,14 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;

import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.google.auto.value.AutoValue;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableSortedMap;
import com.google.devtools.build.lib.actions.ActionEnvironment;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.analysis.config.FragmentClassSet;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.FragmentFactory;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException;
import com.google.devtools.build.lib.cmdline.RepositoryName;
Expand All @@ -36,21 +30,14 @@
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.lang.reflect.InvocationTargetException;
import java.util.Set;
import java.util.concurrent.CompletionException;
import net.starlark.java.eval.StarlarkSemantics;

/** A builder for {@link BuildConfigurationValue} instances. */
public final class BuildConfigurationFunction implements SkyFunction {

/** Cache with weak values can't have null values. */
private static final Fragment NULL_MARKER = new Fragment() {};

private final BlazeDirectories directories;
private final ConfiguredRuleClassProvider ruleClassProvider;
private final LoadingCache<FragmentKey, Fragment> fragmentCache =
Caffeine.newBuilder().weakValues().build(BuildConfigurationFunction::makeFragment);
private final FragmentFactory fragmentFactory = new FragmentFactory();

public BuildConfigurationFunction(
BlazeDirectories directories, RuleClassProvider ruleClassProvider) {
Expand Down Expand Up @@ -104,73 +91,14 @@ private ImmutableSortedMap<Class<? extends Fragment>, Fragment> getConfiguration
ImmutableSortedMap.Builder<Class<? extends Fragment>, Fragment> fragments =
ImmutableSortedMap.orderedBy(FragmentClassSet.LEXICAL_FRAGMENT_SORTER);
for (Class<? extends Fragment> fragmentClass : fragmentClasses) {
BuildOptions trimmedOptions = trimToRequiredOptions(key.getOptions(), fragmentClass);
Fragment fragment;
FragmentKey fragmentKey = FragmentKey.create(trimmedOptions, fragmentClass);
try {
fragment = fragmentCache.get(fragmentKey);
} catch (CompletionException e) {
Throwables.propagateIfPossible(e.getCause(), InvalidConfigurationException.class);
throw e;
}
if (fragment != NULL_MARKER) {
Fragment fragment = fragmentFactory.createFragment(key.getOptions(), fragmentClass);
if (fragment != null) {
fragments.put(fragmentClass, fragment);
} else {
// NULL_MARKER is never GC'ed, so this entry will stay in cache forever unless we delete it
// ourselves. Since it's a cheap computation we don't care about recomputing it.
fragmentCache.invalidate(fragmentKey);
}
}
return fragments.build();
}

private static BuildOptions trimToRequiredOptions(
BuildOptions original, Class<? extends Fragment> fragment) {
BuildOptions.Builder trimmed = BuildOptions.builder();
Set<Class<? extends FragmentOptions>> requiredOptions = Fragment.requiredOptions(fragment);
for (FragmentOptions options : original.getNativeOptions()) {
// CoreOptions is implicitly required by all fragments.
if (options instanceof CoreOptions || requiredOptions.contains(options.getClass())) {
trimmed.addFragmentOptions(options);
}
}
if (Fragment.requiresStarlarkOptions(fragment)) {
trimmed.addStarlarkOptions(original.getStarlarkOptions());
}
return trimmed.build();
}

@AutoValue
abstract static class FragmentKey {
abstract BuildOptions getBuildOptions();

abstract Class<? extends Fragment> getFragmentClass();

private static FragmentKey create(
BuildOptions buildOptions, Class<? extends Fragment> fragmentClass) {
return new AutoValue_BuildConfigurationFunction_FragmentKey(buildOptions, fragmentClass);
}
}

private static Fragment makeFragment(FragmentKey fragmentKey)
throws InvalidConfigurationException {
BuildOptions buildOptions = fragmentKey.getBuildOptions();
Class<? extends Fragment> fragmentClass = fragmentKey.getFragmentClass();
String noConstructorPattern = "%s lacks constructor(BuildOptions)";
try {
Fragment fragment =
fragmentClass.getConstructor(BuildOptions.class).newInstance(buildOptions);
return fragment.shouldInclude() ? fragment : NULL_MARKER;
} catch (InvocationTargetException e) {
if (e.getCause() instanceof InvalidConfigurationException) {
throw (InvalidConfigurationException) e.getCause();
}
throw new IllegalStateException(String.format(noConstructorPattern, fragmentClass), e);
} catch (ReflectiveOperationException e) {
throw new IllegalStateException(String.format(noConstructorPattern, fragmentClass), e);
}
}

@Override
public String extractTag(SkyKey skyKey) {
return null;
Expand Down

0 comments on commit af15543

Please sign in to comment.