Simplify class and var names related to bzl skyframe inlining
Also clean up some documentation and test names. A follow-up will restructure the inlining logic.
This is refactoring work toward adding a new kind of .bzl loading context, for Bazel-internal .bzl files.
Work toward #11437.
BzlLoadFunction:
- delete "self" from inlining terminology
- shorten method names e.g. "createForInlining" with no suffix specifying clients
- "visitedDepsInToplevelLoad" -> "visitedBzls", "visitedNested" -> "loadStack"
- add some explanatory comments
- hide the builder-factory detail in the inlining manager
CachedBzlLoadValueAndDeps:
- rename to "CachedBzlLoadData", with local instances called "cachedData"
- rename its builder factory
- have traverse() process the root node as well (shreyax@ sees no issue with this)
Other
- rename test cases in BzlLoadFunctionTest
RELNOTES: None
PiperOrigin-RevId: 313428699
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 649cf3f..35d1310 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -943,7 +943,7 @@
java_library(
name = "cached_bzl_load_value_and_deps",
- srcs = ["CachedBzlLoadValueAndDeps.java"],
+ srcs = ["CachedBzlLoadData.java"],
deps = [
":bzl_load_value",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
@@ -954,7 +954,7 @@
java_library(
name = "cached_bzl_load_value_and_deps_builder_factory",
- srcs = ["CachedBzlLoadValueAndDepsBuilderFactory.java"],
+ srcs = ["CachedBzlLoadDataBuilderFactory.java"],
deps = [
":cached_bzl_load_value_and_deps",
"//src/main/java/com/google/devtools/build/lib/concurrent",
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 7cf5f65..6c53e18 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
@@ -87,7 +87,7 @@
private final PackageFactory packageFactory;
private final ASTFileLookupValueManager astFileLookupValueManager;
- @Nullable private final SelfInliningManager selfInliningManager;
+ @Nullable private final InliningManager inliningManager;
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
@@ -95,11 +95,11 @@
RuleClassProvider ruleClassProvider,
PackageFactory packageFactory,
ASTFileLookupValueManager astFileLookupValueManager,
- @Nullable SelfInliningManager selfInliningManager) {
+ @Nullable InliningManager inliningManager) {
this.ruleClassProvider = ruleClassProvider;
this.packageFactory = packageFactory;
this.astFileLookupValueManager = astFileLookupValueManager;
- this.selfInliningManager = selfInliningManager;
+ this.inliningManager = inliningManager;
}
public static BzlLoadFunction create(
@@ -135,10 +135,10 @@
// waste.
new InliningAndCachingASTFileLookupValueManager(
ruleClassProvider, digestHashFunction, astFileLookupValueCache),
- /*selfInliningManager=*/ null);
+ /*inliningManager=*/ null);
}
- public static BzlLoadFunction createForInliningSelfForPackageAndWorkspaceNodes(
+ public static BzlLoadFunction createForInlining(
RuleClassProvider ruleClassProvider,
PackageFactory packageFactory,
int bzlLoadValueCacheSize) {
@@ -151,7 +151,7 @@
// of a BzlLoadValue inlining cache miss). This is important in the situation where a bzl
// file is loaded by a lot of other bzl files or BUILD files.
RegularSkyframeASTFileLookupValueManager.INSTANCE,
- new SelfInliningManager(bzlLoadValueCacheSize));
+ new InliningManager(bzlLoadValueCacheSize));
}
@Override
@@ -168,39 +168,48 @@
}
}
+ /**
+ * Entry point for computing "inline", without any direct or indirect Skyframe calls back into
+ * {@link BzlLoadFunction}. (Other Skyframe calls are permitted.)
+ *
+ * <p>{@code visitedBzls} is a cache of values previously computed, and will be added to by this
+ * call.
+ */
@Nullable
- BzlLoadValue computeWithSelfInlineCallsForPackageAndWorkspaceNodes(
- BzlLoadValue.Key key,
- Environment env,
- Map<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> visitedDepsInToplevelLoad)
+ BzlLoadValue computeInline(
+ BzlLoadValue.Key key, Environment env, Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls)
throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
- Preconditions.checkNotNull(selfInliningManager);
- // See comments in computeWithSelfInlineCallsInternal for an explanation of the visitedNested
- // and visitedDepsInToplevelLoad vars.
- CachedBzlLoadValueAndDeps cachedBzlLoadValueAndDeps =
- computeWithSelfInlineCallsInternal(
+ Preconditions.checkNotNull(inliningManager);
+ // See comments in computeInlineInternal for an explanation of the loadStack and visitedBzls
+ // vars.
+ // TODO(brandjon): centralize the explanation of these vars.
+ CachedBzlLoadData cachedData =
+ computeInlineInternal(
key,
env,
- // visitedNested must use insertion order to display the correct error.
- /*visitedNested=*/ new LinkedHashSet<>(),
- /*visitedDepsInToplevelLoad=*/ visitedDepsInToplevelLoad);
- if (cachedBzlLoadValueAndDeps == null) {
- return null;
- }
- return cachedBzlLoadValueAndDeps.getValue();
+ // loadStack must use insertion order to display the correct error.
+ /*loadStack=*/ new LinkedHashSet<>(),
+ /*visitedBzls=*/ visitedBzls);
+ return cachedData != null ? cachedData.getValue() : null;
}
+ /**
+ * Retrieves or creates the requested CachedBzlLoadData object, entering it into the caches.
+ *
+ * <p>Returns null if the underlying Skyvalue computation is null (error or Skyframe restart).
+ */
@Nullable
- private CachedBzlLoadValueAndDeps computeWithSelfInlineCallsInternal(
+ private CachedBzlLoadData computeInlineInternal(
BzlLoadValue.Key key,
Environment env,
- Set<BzlLoadValue.Key> visitedNested,
- Map<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> visitedDepsInToplevelLoad)
+ Set<BzlLoadValue.Key> loadStack,
+ Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls)
throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
- // Under BzlLoadFunction inlining, BUILD and WORKSPACE files are evaluated in separate Skyframe
- // threads, but all the .bzls transitively loaded by a single package occur in one thread. All
- // these threads share a global cache in selfInliningManager, so that once any thread completes
- // evaluation of a .bzl, it needn't be evaluated again (unless it's evicted).
+ // Under BzlLoadFunction inlining, BUILD and WORKSPACE files (and the singular
+ // StarlarkBuiltinsFunction) are evaluated in separate Skyframe threads, but all the .bzls
+ // transitively loaded by a single package occur in one thread. All these threads share a global
+ // cache in inliningManager, so that once any thread completes evaluation of a .bzl, it needn't
+ // be evaluated again (unless it's evicted).
//
// If two threads race to evaluate the same .bzl, each one will see a different copy of it, and
// only one will end up in the global cache. This presents a hazard if the same BUILD or
@@ -213,40 +222,41 @@
// two threads could deadlock while trying to evaluate an illegal load() cycle from opposite
// ends.)
//
- // To solve this, we keep a second cache in visitedDepsInToplevelLoad, of just the .bzls
- // transitively loaded in the current package. The entry for foo.bzl may be a different copy
- // than the one in the global cache, but the BUILD or WORKSPACE file won't know the difference.
- // (We don't need to worry about Starlark values from different packages interacting since
- // inlining is only used for the loading phase.)
+ // To solve this, we keep a second cache in visitedBzls, of just the .bzls transitively loaded
+ // in the current package. The entry for foo.bzl may be a different copy than the one in the
+ // global cache, but the BUILD or WORKSPACE file won't know the difference. (We don't need to
+ // worry about Starlark values from different packages interacting since inlining is only used
+ // for the loading phase.)
//
- CachedBzlLoadValueAndDeps cachedBzlLoadValueAndDeps = visitedDepsInToplevelLoad.get(key);
- if (cachedBzlLoadValueAndDeps == null) {
- cachedBzlLoadValueAndDeps = selfInliningManager.bzlLoadValueCache.getIfPresent(key);
- if (cachedBzlLoadValueAndDeps != null) {
- cachedBzlLoadValueAndDeps.traverse(env::registerDependencies, visitedDepsInToplevelLoad);
+ CachedBzlLoadData cachedData = visitedBzls.get(key);
+ if (cachedData == null) {
+ cachedData = inliningManager.bzlLoadValueCache.getIfPresent(key);
+ if (cachedData != null) {
+ // Found a cache hit from another thread's computation; register the recorded deps as if our
+ // thread required them for the current key. Incorporate into visitedBzls any transitive
+ // cache hits it does not already contain.
+ cachedData.traverse(env::registerDependencies, visitedBzls);
}
}
- if (cachedBzlLoadValueAndDeps != null) {
- return cachedBzlLoadValueAndDeps;
+ if (cachedData != null) {
+ return cachedData;
}
- // visitedNested is keyed on the SkyKey, not the label, because it's possible for distinct keys
- // to share the same label. Examples include the "@builtins" pseudo-repo vs a real repository
- // that happens to be named "@builtins", or keys for the same .bzl with different workspace
- // chunking information. It's unclear whether these particular cycles can arise in practice, but
- // it doesn't hurt to be robust to future changes that may make that possible.
- if (!visitedNested.add(key)) {
+ // loadStack is keyed on the SkyKey, not the label, because it's possible for distinct keys to
+ // share the same label. Examples include the "@builtins" pseudo-repo vs a real repository that
+ // happens to be named "@builtins", or keys for the same .bzl with different workspace chunking
+ // information. It's unclear whether these particular cycles can arise in practice, but it
+ // doesn't hurt to be robust to future changes that may make that possible.
+ if (!loadStack.add(key)) {
ImmutableList<BzlLoadValue.Key> cycle =
- CycleUtils.splitIntoPathAndChain(Predicates.equalTo(key), visitedNested).second;
+ CycleUtils.splitIntoPathAndChain(Predicates.equalTo(key), loadStack).second;
throw new BzlLoadFailedException("Starlark load cycle: " + cycle);
}
- CachedBzlLoadValueAndDeps.Builder inlineCachedValueBuilder =
- selfInliningManager.cachedBzlLoadValueAndDepsBuilderFactory
- .newCachedBzlLoadValueAndDepsBuilder();
- // Use an instrumented Skyframe env to capture Skyframe deps in the CachedBzlLoadValueAndDeps.
- // This is transitive but doesn't include deps underneath recursively loaded .bzls (the
- // recursion uses the unwrapped original env).
+ CachedBzlLoadData.Builder cachedDataBuilder = inliningManager.cachedDataBuilder();
+ // Use an instrumented Skyframe env to capture Skyframe deps in the CachedBzlLoadData. This is
+ // transitive but doesn't include deps underneath recursively loaded .bzls (the recursion uses
+ // the unwrapped original env).
Preconditions.checkState(
!(env instanceof RecordingSkyFunctionEnvironment),
"Found nested RecordingSkyFunctionEnvironment but it should have been stripped: %s",
@@ -254,29 +264,27 @@
RecordingSkyFunctionEnvironment recordingEnv =
new RecordingSkyFunctionEnvironment(
env,
- inlineCachedValueBuilder::addDep,
- inlineCachedValueBuilder::addDeps,
- inlineCachedValueBuilder::noteException);
+ cachedDataBuilder::addDep,
+ cachedDataBuilder::addDeps,
+ cachedDataBuilder::noteException);
BzlLoadValue value =
computeInternal(
- key,
- recordingEnv,
- new InliningState(visitedNested, inlineCachedValueBuilder, visitedDepsInToplevelLoad));
+ key, recordingEnv, new InliningState(loadStack, cachedDataBuilder, visitedBzls));
// All loads traversed, this key can no longer be part of a cycle.
- Preconditions.checkState(visitedNested.remove(key), key);
+ Preconditions.checkState(loadStack.remove(key), key);
if (value != null) {
- inlineCachedValueBuilder.setValue(value);
- inlineCachedValueBuilder.setKey(key);
- cachedBzlLoadValueAndDeps = inlineCachedValueBuilder.build();
- visitedDepsInToplevelLoad.put(key, cachedBzlLoadValueAndDeps);
- selfInliningManager.bzlLoadValueCache.put(key, cachedBzlLoadValueAndDeps);
+ cachedDataBuilder.setValue(value);
+ cachedDataBuilder.setKey(key);
+ cachedData = cachedDataBuilder.build();
+ visitedBzls.put(key, cachedData);
+ inliningManager.bzlLoadValueCache.put(key, cachedData);
}
- return cachedBzlLoadValueAndDeps;
+ return cachedData;
}
- public void resetSelfInliningCache() {
- selfInliningManager.reset();
+ public void resetInliningCache() {
+ inliningManager.reset();
}
private static ContainingPackageLookupValue getContainingPackageLookupValue(
@@ -313,18 +321,22 @@
return containingPackageLookupValue;
}
+ /**
+ * Value class bundling parameters that are only used in the code path where BzlLoadFunction calls
+ * are inlined.
+ */
private static class InliningState {
- private final Set<BzlLoadValue.Key> visitedNested;
- private final CachedBzlLoadValueAndDeps.Builder inlineCachedValueBuilder;
- private final Map<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> visitedDepsInToplevelLoad;
+ private final Set<BzlLoadValue.Key> loadStack;
+ private final CachedBzlLoadData.Builder cachedDataBuilder;
+ private final Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls;
private InliningState(
- Set<BzlLoadValue.Key> visitedNested,
- CachedBzlLoadValueAndDeps.Builder inlineCachedValueBuilder,
- Map<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> visitedDepsInToplevelLoad) {
- this.visitedNested = visitedNested;
- this.inlineCachedValueBuilder = inlineCachedValueBuilder;
- this.visitedDepsInToplevelLoad = visitedDepsInToplevelLoad;
+ Set<BzlLoadValue.Key> loadStack,
+ CachedBzlLoadData.Builder cachedDataBuilder,
+ Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls) {
+ this.loadStack = loadStack;
+ this.cachedDataBuilder = cachedDataBuilder;
+ this.visitedBzls = visitedBzls;
}
}
@@ -417,7 +429,7 @@
List<BzlLoadValue> bzlLoads =
inliningState == null
? computeBzlLoadsNoInlining(env, loadKeys, file.getStartLocation())
- : computeBzlLoadsWithSelfInlining(env, loadKeys, label, inliningState);
+ : computeBzlLoadsWithInlining(env, loadKeys, label, inliningState);
if (bzlLoads == null) {
return null; // Skyframe deps unavailable
}
@@ -555,8 +567,8 @@
}
/**
- * Compute the BzlLoadValue for all given keys using vanilla Skyframe evaluation, returning {@code
- * null} if Skyframe deps were missing and have been requested.
+ * Computes the BzlLoadValue for all given keys using vanilla Skyframe evaluation, returning
+ * {@code null} if Skyframe deps were missing and have been requested.
*/
@Nullable
private static List<BzlLoadValue> computeBzlLoadsNoInlining(
@@ -578,12 +590,12 @@
}
/**
- * Compute the BzlLoadValue for all given keys by reusing this instance of the BzlLoadFunction,
+ * Computes the BzlLoadValue for all given keys by reusing this instance of the BzlLoadFunction,
* bypassing traditional Skyframe evaluation, returning {@code null} if Skyframe deps were missing
* and have been requested.
*/
@Nullable
- private List<BzlLoadValue> computeBzlLoadsWithSelfInlining(
+ private List<BzlLoadValue> computeBzlLoadsWithInlining(
Environment env, List<BzlLoadValue.Key> keys, Label fileLabel, InliningState inliningState)
throws InterruptedException, BzlLoadFailedException, InconsistentFilesystemException {
Preconditions.checkState(
@@ -596,29 +608,26 @@
boolean valuesMissing = false;
// NOTE: Iterating over loads in the order listed in the file.
for (BzlLoadValue.Key key : keys) {
- CachedBzlLoadValueAndDeps cachedValue;
+ CachedBzlLoadData cachedData;
try {
- cachedValue =
- computeWithSelfInlineCallsInternal(
- key,
- strippedEnv,
- inliningState.visitedNested,
- inliningState.visitedDepsInToplevelLoad);
+ cachedData =
+ computeInlineInternal(
+ key, strippedEnv, inliningState.loadStack, inliningState.visitedBzls);
} 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.
deferredException = MoreObjects.firstNonNull(deferredException, e);
continue;
}
- if (cachedValue == null) {
+ if (cachedData == null) {
Preconditions.checkState(env.valuesMissing(), "no starlark load value for %s", key);
// We continue making inline calls even if some requested values are missing, to maximize
// the number of dependent (non-inlined) SkyFunctions that are requested, thus avoiding a
// quadratic number of restarts.
valuesMissing = true;
} else {
- bzlLoads.add(cachedValue.getValue());
- inliningState.inlineCachedValueBuilder.addTransitiveDeps(cachedValue);
+ bzlLoads.add(cachedData.getValue());
+ inliningState.cachedDataBuilder.addTransitiveDeps(cachedData);
}
}
if (deferredException != null) {
@@ -830,22 +839,27 @@
}
}
- private static class SelfInliningManager {
+ /** Per-instance manager for when {@code BzlLoadFunction} calls are inlined. */
+ private static class InliningManager {
private final int bzlLoadValueCacheSize;
- private Cache<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> bzlLoadValueCache;
- private CachedBzlLoadValueAndDepsBuilderFactory cachedBzlLoadValueAndDepsBuilderFactory =
- new CachedBzlLoadValueAndDepsBuilderFactory();
+ private Cache<BzlLoadValue.Key, CachedBzlLoadData> bzlLoadValueCache;
+ private CachedBzlLoadDataBuilderFactory cachedBzlLoadDataBuilderFactory =
+ new CachedBzlLoadDataBuilderFactory();
- private SelfInliningManager(int bzlLoadValueCacheSize) {
+ private InliningManager(int bzlLoadValueCacheSize) {
this.bzlLoadValueCacheSize = bzlLoadValueCacheSize;
}
+ private CachedBzlLoadData.Builder cachedDataBuilder() {
+ return cachedBzlLoadDataBuilderFactory.newCachedBzlLoadDataBuilder();
+ }
+
private void reset() {
if (bzlLoadValueCache != null) {
logger.atInfo().log(
"Starlark inlining cache stats from earlier build: " + bzlLoadValueCache.stats());
}
- cachedBzlLoadValueAndDepsBuilderFactory = new CachedBzlLoadValueAndDepsBuilderFactory();
+ cachedBzlLoadDataBuilderFactory = new CachedBzlLoadDataBuilderFactory();
Preconditions.checkState(
bzlLoadValueCacheSize >= 0,
"Expected positive Starlark cache size if caching. %s",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java
index 5d520c1..5f9347b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java
@@ -61,7 +61,7 @@
return transitiveDigest;
}
- /** Returns the immediate Starlark file dependency corresponding to this load value. */
+ /** Returns the root of a DAG whose structure mirrors the transitive loads of this file. */
public StarlarkFileDependency getDependency() {
return dependency;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadValueAndDeps.java b/src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadData.java
similarity index 70%
rename from src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadValueAndDeps.java
rename to src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadData.java
index 7c9b469..e2d250b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadValueAndDeps.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadData.java
@@ -25,35 +25,48 @@
import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;
-class CachedBzlLoadValueAndDeps {
+/**
+ * A saved {@link BzlLoadFunction} computation, used when inlining that Skyfunction.
+ *
+ * <p>This holds a requested key, its computed value, and the direct and transitive Skyframe
+ * dependencies that are needed to compute it from scratch (i.e. if it weren't cached). Here
+ * "transitive" means "underneath another {@code BzlLoadFunction} computation"; we split them into
+ * other {@code CachedBzlLoadData} objects so they can be shared by other requesting bzls.
+ */
+class CachedBzlLoadData {
private final BzlLoadValue.Key key;
private final BzlLoadValue value;
private final ImmutableList<Iterable<SkyKey>> directDeps;
- private final ImmutableList<CachedBzlLoadValueAndDeps> transitiveDeps;
+ private final ImmutableList<CachedBzlLoadData> transitiveDeps;
- private CachedBzlLoadValueAndDeps(
+ private CachedBzlLoadData(
BzlLoadValue.Key key,
BzlLoadValue value,
ImmutableList<Iterable<SkyKey>> directDeps,
- ImmutableList<CachedBzlLoadValueAndDeps> transitiveDeps) {
+ ImmutableList<CachedBzlLoadData> transitiveDeps) {
this.key = key;
this.value = value;
this.directDeps = directDeps;
this.transitiveDeps = transitiveDeps;
}
+ /**
+ * Adds all deps (direct and transitive) of this value to the {@code visitedDeps} set and passes
+ * them to the consumer (with unspecified order and grouping). The traversal does not include
+ * nodes already contained in {@code visitedDeps}.
+ */
void traverse(
- DepGroupConsumer depGroupConsumer,
- Map<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> visitedDeps)
+ DepGroupConsumer depGroupConsumer, Map<BzlLoadValue.Key, CachedBzlLoadData> visitedDeps)
throws InterruptedException {
+ if (visitedDeps.putIfAbsent(key, this) != null) {
+ return;
+ }
+
for (Iterable<SkyKey> directDepGroup : directDeps) {
depGroupConsumer.accept(directDepGroup);
}
- for (CachedBzlLoadValueAndDeps indirectDeps : transitiveDeps) {
- if (!visitedDeps.containsKey(indirectDeps.key)) {
- visitedDeps.put(indirectDeps.key, indirectDeps);
- indirectDeps.traverse(depGroupConsumer, visitedDeps);
- }
+ for (CachedBzlLoadData indirectDeps : transitiveDeps) {
+ indirectDeps.traverse(depGroupConsumer, visitedDeps);
}
}
@@ -63,10 +76,10 @@
@Override
public boolean equals(Object obj) {
- if (obj instanceof CachedBzlLoadValueAndDeps) {
+ if (obj instanceof CachedBzlLoadData) {
// With the interner, force there to be exactly one cached value per key at any given point
// in time.
- return this.key.equals(((CachedBzlLoadValueAndDeps) obj).key);
+ return this.key.equals(((CachedBzlLoadData) obj).key);
}
return false;
}
@@ -82,13 +95,13 @@
}
static class Builder {
- Builder(Interner<CachedBzlLoadValueAndDeps> interner) {
+ Builder(Interner<CachedBzlLoadData> interner) {
this.interner = interner;
}
- private final Interner<CachedBzlLoadValueAndDeps> interner;
+ private final Interner<CachedBzlLoadData> interner;
private final List<Iterable<SkyKey>> directDeps = new ArrayList<>();
- private final List<CachedBzlLoadValueAndDeps> transitiveDeps = new ArrayList<>();
+ private final List<CachedBzlLoadData> transitiveDeps = new ArrayList<>();
private final AtomicReference<Exception> exceptionSeen = new AtomicReference<>(null);
private BzlLoadValue value;
private BzlLoadValue.Key key;
@@ -116,7 +129,7 @@
}
@CanIgnoreReturnValue
- Builder addTransitiveDeps(CachedBzlLoadValueAndDeps transitiveDeps) {
+ Builder addTransitiveDeps(CachedBzlLoadData transitiveDeps) {
this.transitiveDeps.add(transitiveDeps);
return this;
}
@@ -133,19 +146,19 @@
return this;
}
- CachedBzlLoadValueAndDeps build() {
+ CachedBzlLoadData build() {
// We expect that we don't handle any exceptions in BzlLoadFunction directly.
Preconditions.checkState(exceptionSeen.get() == null, "Caching a value in error?: %s", this);
Preconditions.checkNotNull(value, "Expected value to be set: %s", this);
Preconditions.checkNotNull(key, "Expected key to be set: %s", this);
return interner.intern(
- new CachedBzlLoadValueAndDeps(
+ new CachedBzlLoadData(
key, value, ImmutableList.copyOf(directDeps), ImmutableList.copyOf(transitiveDeps)));
}
@Override
public String toString() {
- return MoreObjects.toStringHelper(CachedBzlLoadValueAndDeps.Builder.class)
+ return MoreObjects.toStringHelper(CachedBzlLoadData.Builder.class)
.add("key", key)
.add("value", value)
.add("directDeps", directDeps)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadValueAndDepsBuilderFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadDataBuilderFactory.java
similarity index 62%
rename from src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadValueAndDepsBuilderFactory.java
rename to src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadDataBuilderFactory.java
index 77fc055..6b82e11 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadValueAndDepsBuilderFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadDataBuilderFactory.java
@@ -17,11 +17,13 @@
import com.google.common.collect.Interner;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
-/** Factory class for producing CachedBzlLoadValueAndDeps. */
-public class CachedBzlLoadValueAndDepsBuilderFactory {
- private final Interner<CachedBzlLoadValueAndDeps> interner = BlazeInterners.newWeakInterner();
+/** Factory class for producing a builder for {@link CachedBzlLoadData}. */
+// TODO(bazel-team): It's unclear why this needs to be public and whether we need it at all. Maybe
+// we can just inline it into BzlLoadFunction.
+public class CachedBzlLoadDataBuilderFactory {
+ private final Interner<CachedBzlLoadData> interner = BlazeInterners.newWeakInterner();
- CachedBzlLoadValueAndDeps.Builder newCachedBzlLoadValueAndDepsBuilder() {
- return new CachedBzlLoadValueAndDeps.Builder(interner);
+ CachedBzlLoadData.Builder newCachedBzlLoadDataBuilder() {
+ return new CachedBzlLoadData.Builder(interner);
}
}
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 2d1c5e7..7d6b845 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
@@ -679,14 +679,12 @@
// .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, CachedBzlLoadValueAndDeps> visitedDepsInToplevelLoad = new HashMap<>();
+ Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls = new HashMap<>();
for (BzlLoadValue.Key key : keys) {
SkyValue skyValue;
try {
- // Will complete right away if it's already cached in visitedDepsInToplevelLoad.
- skyValue =
- bzlLoadFunctionForInlining.computeWithSelfInlineCallsForPackageAndWorkspaceNodes(
- key, env, visitedDepsInToplevelLoad);
+ // Will complete right away if it's already cached in visitedBzls.
+ skyValue = bzlLoadFunctionForInlining.computeInline(key, env, visitedBzls);
} 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.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkFileDependency.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkFileDependency.java
index 94c513e..e53252b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkFileDependency.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkFileDependency.java
@@ -19,12 +19,7 @@
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import java.util.Objects;
-/**
- * A simple value class to store the direct Starlark file dependencies of a Starlark extension file.
- * It also contains a Label identifying the extension file.
- *
- * <p>The dependency structure must be acyclic.
- */
+/** A value class representing a node in a DAG that mirrors the load graph of a Starlark file. */
@AutoCodec
public class StarlarkFileDependency {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java
index e060a07..060831b 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java
@@ -267,7 +267,7 @@
}
@Test
- public void testLoadUsingLabelThatDoesntCrossBoundaryOfPackage() throws Exception {
+ public void testLoadFromSubdirInSamePackageIsOk() throws Exception {
scratch.file("a/BUILD");
scratch.file("a/a.bzl", "load('//a:b/b.bzl', 'b')");
scratch.file("a/b/b.bzl", "b = 42");
@@ -276,7 +276,7 @@
}
@Test
- public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Disallow_OfSamePkg() throws Exception {
+ public void testLoadMustRespectPackageBoundary_ofSubpkg() throws Exception {
scratch.file("a/BUILD");
scratch.file("a/a.bzl", "load('//a:b/b.bzl', 'b')");
scratch.file("a/b/BUILD", "");
@@ -288,8 +288,7 @@
}
@Test
- public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Disallow_OfSamePkg_Relative()
- throws Exception {
+ public void testLoadMustRespectPackageBoundary_ofSubpkg_relative() throws Exception {
scratch.file("a/BUILD");
scratch.file("a/a.bzl", "load('b/b.bzl', 'b')");
scratch.file("a/b/BUILD", "");
@@ -301,8 +300,7 @@
}
@Test
- public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Disallow_OfDifferentPkgUnder()
- throws Exception {
+ public void testLoadMustRespectPackageBoundary_ofIndirectSubpkg() throws Exception {
scratch.file("a/BUILD");
scratch.file("a/a.bzl", "load('//a/b:c/c.bzl', 'c')");
scratch.file("a/b/BUILD", "");
@@ -315,8 +313,7 @@
}
@Test
- public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Disallow_OfDifferentPkgAbove()
- throws Exception {
+ public void testLoadMustRespectPackageBoundary_ofParentPkg() throws Exception {
scratch.file("a/b/BUILD");
scratch.file("a/b/b.bzl", "load('//a/c:c/c.bzl', 'c')");
scratch.file("a/BUILD");
@@ -347,8 +344,7 @@
}
@Test
- public void testWithNonExistentRepository_And_DisallowLoadUsingLabelThatCrossesBoundaryOfPackage()
- throws Exception {
+ public void testLoadFromNonExistentRepository_producesMeaningfulError() throws Exception {
scratch.file("BUILD", "load(\"@repository//dir:file.bzl\", \"foo\")");
SkyKey skyKey = key("@repository//dir:file.bzl");
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadValueAndDepsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadDataTest.java
similarity index 75%
rename from src/test/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadValueAndDepsTest.java
rename to src/test/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadDataTest.java
index 24370e9..e561c99 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadValueAndDepsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/CachedBzlLoadDataTest.java
@@ -28,9 +28,9 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/** Tests for {@link CachedBzlLoadValueAndDeps}. */
+/** Tests for {@link CachedBzlLoadData}. */
@RunWith(JUnit4.class)
-public class CachedBzlLoadValueAndDepsTest {
+public class CachedBzlLoadDataTest {
@Test
public void testDepsAreNotVisitedMultipleTimesForDiamondDependencies() throws Exception {
// Graph structure of BzlLoadValues:
@@ -42,16 +42,16 @@
// gc
BzlLoadValue dummyValue = mock(BzlLoadValue.class);
- CachedBzlLoadValueAndDepsBuilderFactory cachedBzlLoadValueAndDepsBuilderFactory =
- new CachedBzlLoadValueAndDepsBuilderFactory();
+ CachedBzlLoadDataBuilderFactory cachedBzlLoadDataBuilderFactory =
+ new CachedBzlLoadDataBuilderFactory();
BzlLoadValue.Key gcKey = createStarlarkKey("//gc");
SkyKey gcKey1 = createKey("gc key1");
SkyKey gcKey2 = createKey("gc key2");
SkyKey gcKey3 = createKey("gc key3");
- CachedBzlLoadValueAndDeps gc =
- cachedBzlLoadValueAndDepsBuilderFactory
- .newCachedBzlLoadValueAndDepsBuilder()
+ CachedBzlLoadData gc =
+ cachedBzlLoadDataBuilderFactory
+ .newCachedBzlLoadDataBuilder()
.addDep(gcKey1)
.addDeps(ImmutableList.of(gcKey2, gcKey3))
.setKey(gcKey)
@@ -60,9 +60,9 @@
BzlLoadValue.Key c1Key = createStarlarkKey("//c1");
SkyKey c1Key1 = createKey("c1 key1");
- CachedBzlLoadValueAndDeps c1 =
- cachedBzlLoadValueAndDepsBuilderFactory
- .newCachedBzlLoadValueAndDepsBuilder()
+ CachedBzlLoadData c1 =
+ cachedBzlLoadDataBuilderFactory
+ .newCachedBzlLoadDataBuilder()
.addDep(c1Key1)
.addTransitiveDeps(gc)
.setValue(dummyValue)
@@ -72,9 +72,9 @@
BzlLoadValue.Key c2Key = createStarlarkKey("//c2");
SkyKey c2Key1 = createKey("c2 key1");
SkyKey c2Key2 = createKey("c2 key2");
- CachedBzlLoadValueAndDeps c2 =
- cachedBzlLoadValueAndDepsBuilderFactory
- .newCachedBzlLoadValueAndDepsBuilder()
+ CachedBzlLoadData c2 =
+ cachedBzlLoadDataBuilderFactory
+ .newCachedBzlLoadDataBuilder()
.addDeps(ImmutableList.of(c2Key1, c2Key2))
.addTransitiveDeps(gc)
.setValue(dummyValue)
@@ -83,9 +83,9 @@
BzlLoadValue.Key pKey = createStarlarkKey("//p");
SkyKey pKey1 = createKey("p key1");
- CachedBzlLoadValueAndDeps p =
- cachedBzlLoadValueAndDepsBuilderFactory
- .newCachedBzlLoadValueAndDepsBuilder()
+ CachedBzlLoadData p =
+ cachedBzlLoadDataBuilderFactory
+ .newCachedBzlLoadDataBuilder()
.addDep(pKey1)
.addTransitiveDeps(c1)
.addTransitiveDeps(c2)
@@ -94,8 +94,8 @@
.build();
List<Iterable<SkyKey>> registeredDeps = new ArrayList<>();
- Map<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> visitedDepsInToplevelLoad = new HashMap<>();
- p.traverse(registeredDeps::add, visitedDepsInToplevelLoad);
+ Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls = new HashMap<>();
+ p.traverse(registeredDeps::add, visitedBzls);
assertThat(registeredDeps)
.containsExactly(
@@ -106,8 +106,7 @@
ImmutableList.of(c2Key1, c2Key2))
.inOrder();
- // Note that (pKey, p) is expected to be added separately.
- assertThat(visitedDepsInToplevelLoad).containsExactly(c1Key, c1, c2Key, c2, gcKey, gc);
+ assertThat(visitedBzls).containsExactly(pKey, p, c1Key, c1, c2Key, c2, gcKey, gc);
}
private static SkyKey createKey(String name) {
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkIntegrationTest.java
index 7a7d311..7b6d2e6 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkIntegrationTest.java
@@ -2953,9 +2953,9 @@
((InMemoryMemoizingEvaluator) getSkyframeExecutor().getEvaluatorForTesting())
.getSkyFunctionsForTesting();
BzlLoadFunction bzlLoadFunction =
- BzlLoadFunction.createForInliningSelfForPackageAndWorkspaceNodes(
+ BzlLoadFunction.createForInlining(
this.getRuleClassProvider(), this.getPackageFactory(), /*bzlLoadValueCacheSize=*/ 2);
- bzlLoadFunction.resetSelfInliningCache();
+ bzlLoadFunction.resetInliningCache();
((PackageFunction) skyFunctions.get(SkyFunctions.PACKAGE))
.setBzlLoadFunctionForInliningForTesting(bzlLoadFunction);
}
