Factor BzlLoadFunction.InliningState into callers of the inlining code path
InliningState becomes package-visible and gains some documentation. It is now an opaque object that replaces the set of visited bzl keys for a BUILD file.
This prepares the way for a follow-up CL that has StarlarkBuiltinsFunction call BzlLoadFunction's inlining code path with an existing InliningState (obtained from prior BzlLoadFunction evaluation). It allows us to have it call computeInline(), the same entry point which PackageFunction calls, rather than exposing computeInlineWithState() to package visibility.
computeInlineWithState() is renamed to computeInlineCachedData(), since its distinguishing feature is now only that it returns the cached data wrapper, and not that it accepts InliningState.
Work toward #11437.
RELNOTES: None
PiperOrigin-RevId: 315309686
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
index 9692879..243b1cc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -63,6 +63,7 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.ValueOrException;
+import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
@@ -199,8 +200,9 @@
* {@link BzlLoadFunction}. (Other Skyframe calls are permitted.)
*
* <p><b>USAGE NOTE:</b> This function is intended to be called from {@link PackageFunction} and
- * probably shouldn't be used anywhere else. If you think you need inline Starlark computation,
- * consult with the Core subteam and check out cl/305127325 for an example of correcting a misuse.
+ * {@link StarlarkBuiltinsFunction} and probably shouldn't be used anywhere else. If you think you
+ * need inline Starlark computation, consult with the Core subteam and check out cl/305127325 for
+ * an example of correcting a misuse.
*
* <p>Under bzl inlining, there is some calling context that wants to obtain a set of {@link
* BzlLoadValue}s without Skyframe evaluation. For example, a calling context can be a BUILD file
@@ -218,11 +220,13 @@
* weren't concerned about racing, A may also reevaluate previously computed items due to cache
* evictions.
*
- * <p>To solve this, we keep a second cache, {@code visitedBzls}, that is local to the current
- * calling context, and which never evicts entries. This cache is always checked in preference to
- * the shared one; it may deviate from the shared one in some of its entries, but the calling
- * context won't know the difference. (If bzl inlining is only used for the loading phase, we
- * don't need to worry about Starlark values from different packages interacting.)
+ * <p>To solve this, we keep a second cache, {@link InliningState#visitedBzls}, that is local to
+ * the current calling context, and which never evicts entries. This cache is always checked in
+ * preference to the shared one; it may deviate from the shared one in some of its entries, but
+ * the calling context won't know the difference. (If bzl inlining is only used for the loading
+ * phase, we don't need to worry about Starlark values from different packages interacting.) The
+ * cache is stored as part of the {@code inliningState} passed in by the caller; the caller can
+ * obtain this object using {@link InliningState#create}.
*
* <p>As an aside, note that we can't avoid having a second cache by simply naively blocking
* evaluation of .bzls on retrievals from the shared cache. This is because two contexts could
@@ -235,14 +239,12 @@
* @return the requested {@code BzlLoadValue}, or null if there is a Skyframe restart or error
*/
@Nullable
- BzlLoadValue computeInline(
- BzlLoadValue.Key key, Environment env, Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls)
+ BzlLoadValue computeInline(BzlLoadValue.Key key, Environment env, InliningState inliningState)
throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
// Note to refactorors: No Skyframe calls may be made before the RecordingSkyFunctionEnvironment
// is set up below in computeInlineForCacheMiss.
Preconditions.checkNotNull(cachedBzlLoadDataManager);
- CachedBzlLoadData cachedData =
- computeInlineWithState(key, env, InliningState.createInitial(visitedBzls));
+ CachedBzlLoadData cachedData = computeInlineCachedData(key, env, inliningState);
return cachedData != null ? cachedData.getValue() : null;
}
@@ -251,12 +253,14 @@
* local and shared caches. This is the entry point for recursive calls to the inline code path.
*
* <p>Skyframe calls made underneath this function will be logged in the resulting {@code
- * CachedBzlLoadData) (or its transitive dependencies).
+ * CachedBzlLoadData) (or its transitive dependencies). The given Skyframe environment must not
+ * be a {@link RecordingSkyFunctionEnvironment}, since that would imply that calls are being
+ * logged in both the returned value and the parent value.
*
* <p>Returns null on Skyframe restart or error.
*/
@Nullable
- private CachedBzlLoadData computeInlineWithState(
+ private CachedBzlLoadData computeInlineCachedData(
BzlLoadValue.Key key, Environment env, InliningState inliningState)
throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
// Note to refactorors: No Skyframe calls may be made before the RecordingSkyFunctionEnvironment
@@ -298,7 +302,7 @@
// We use an instrumented Skyframe env to capture Skyframe deps in the CachedBzlLoadData. This
// generally includes transitive Skyframe deps, but specifically excludes deps underneath
// recursively loaded .bzls. We unwrap the instrumented env right before recursively calling
- // back into computeInlineWithState.
+ // back into computeInlineCachedData.
CachedBzlLoadData.Builder cachedDataBuilder = cachedBzlLoadDataManager.cachedDataBuilder();
Preconditions.checkState(
!(env instanceof RecordingSkyFunctionEnvironment),
@@ -393,10 +397,22 @@
}
/**
- * Value class bundling parameters that are only used in the code path where {@code
- * BzlLoadFunction} calls are inlined.
+ * An opaque object that holds state for the inlining computation initiated by {@link
+ * #computeInline}.
+ *
+ * <p>An original caller of {@code computeInline} (e.g., {@link PackageFunction}) should obtain
+ * one of these objects using {@link InliningState#create}. When the same caller makes several
+ * calls to {@code computeInline} (e.g., for multiple top-level loads in the same BUILD file), the
+ * same object must be passed to each call.
+ *
+ * <p>When a Skyfunction that is called by {@code BzlLoadFunction}'s inlining code path in turn
+ * calls back into {@code computeInline}, it should forward along the same {@code InliningState}
+ * that it received. In particular, {@link StarlarkBuiltinsFunction} forwards the inlining state
+ * to ensure that 1) the .bzls that get loaded from the {@code @builtins} pseudo-repository are
+ * properly recorded as dependencies of all .bzl files that use builtins injection, and 2) the
+ * {@code @builtins} .bzls are not reevaluated.
*/
- private static class InliningState {
+ static class InliningState {
/**
* The set of .bzls we're currently in the process of loading. This is used for cycle detection
* since we don't have the benefit of Skyframe's internal cycle detection. The set must use
@@ -422,19 +438,23 @@
this.visitedBzls = visitedBzls;
}
- static InliningState createInitial(Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls) {
+ /**
+ * Creates an initial {@code InliningState} with no information about previously loaded files
+ * (except the shared cache stored in {@link BzlLoadFunction}).
+ */
+ static InliningState create() {
return new InliningState(
- new LinkedHashSet<>(),
- x -> {}, // No parent value to mutate.
- visitedBzls);
+ /*loadStack=*/ new LinkedHashSet<>(),
+ /*childCachedDataHandler=*/ x -> {}, // No parent value to mutate.
+ /*visitedBzls=*/ new HashMap<>());
}
- InliningState createChildState(Consumer<CachedBzlLoadData> childCachedDataHandler) {
+ private InliningState createChildState(Consumer<CachedBzlLoadData> childCachedDataHandler) {
return new InliningState(loadStack, childCachedDataHandler, visitedBzls);
}
/** Records entry to a {@code load()}, throwing an exception if a cycle is detected. */
- void beginLoad(BzlLoadValue.Key key) throws BzlLoadFailedException {
+ private void beginLoad(BzlLoadValue.Key key) throws BzlLoadFailedException {
if (!loadStack.add(key)) {
ImmutableList<BzlLoadValue.Key> cycle =
CycleUtils.splitIntoPathAndChain(Predicates.equalTo(key), loadStack).second;
@@ -443,7 +463,7 @@
}
/** Records exit from a {@code load()}. */
- void finishLoad(BzlLoadValue.Key key) throws BzlLoadFailedException {
+ private void finishLoad(BzlLoadValue.Key key) throws BzlLoadFailedException {
Preconditions.checkState(loadStack.remove(key), key);
}
}
@@ -751,7 +771,7 @@
for (BzlLoadValue.Key key : keys) {
CachedBzlLoadData cachedData;
try {
- cachedData = computeInlineWithState(key, strippedEnv, inliningState);
+ cachedData = computeInlineCachedData(key, strippedEnv, inliningState);
} catch (BzlLoadFailedException e) {
e = BzlLoadFailedException.whileLoadingDep(filePathForErrors, e);
deferredException = MoreObjects.firstNonNull(deferredException, e);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 7d6b845..9d4cd18 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -675,16 +675,16 @@
List<BzlLoadValue> bzlLoads = Lists.newArrayListWithExpectedSize(keys.size());
Exception deferredException = null;
boolean valuesMissing = false;
- // Compute BzlLoadValue for each key, sharing this map as one big cache. This ensures that each
- // .bzl is loaded only once, regardless of diamond dependencies. (Multiple loads of the same
- // .bzl would screw up identity equality of some Starlark symbols -- see comments in
- // BzlLoadFunction.)
- Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls = new HashMap<>();
+ // Compute BzlLoadValue for each key, sharing the same inlining state, i.e. cache of loaded
+ // modules. This ensures that each .bzl is loaded only once, regardless of diamond dependencies
+ // or cache eviction. (Multiple loads of the same .bzl would screw up identity equality of some
+ // Starlark symbols -- see comments in BzlLoadFunction#computeInline.)
+ BzlLoadFunction.InliningState inliningState = BzlLoadFunction.InliningState.create();
for (BzlLoadValue.Key key : keys) {
SkyValue skyValue;
try {
- // Will complete right away if it's already cached in visitedBzls.
- skyValue = bzlLoadFunctionForInlining.computeInline(key, env, visitedBzls);
+ // Will complete right away if it's already cached in inliningState.
+ skyValue = bzlLoadFunctionForInlining.computeInline(key, env, inliningState);
} catch (BzlLoadFailedException | InconsistentFilesystemException e) {
// For determinism's sake while inlining, preserve the first exception and continue to run
// subsequently listed loads to completion/exception, loading all transitive deps anyway.
