Tighten the types of some of the machinery surrounding .bzl skyframe inlining Tighten SkyKey to StarlarkImportLookupValue.Key for the internal functions of StarlarkImportLookupFunction and its inlined (non-Skyframe) caller. This avoids all but one downcast to [...].Key, in compute(), as is expected. Note the naming convention for vars -- "SkyKey skyKey", but "StarlarkImportLookupFunction.Key key". Also eliminate a redundant case in PackageFunction#computeStarlarkImportMapWithInlining, at the expense of making the quick return path not self-evident; added a comment to compensate. This is refactoring work toward adding a new kind of .bzl loading context, for Bazel-internal .bzl files. Work toward #11437. RELNOTES: None PiperOrigin-RevId: 312840745
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 cca5d99..266be0c 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
@@ -648,8 +648,8 @@ } /** - * Compute the StarlarkImportLookupValue for all given SkyKeys using vanilla skyframe evaluation, - * returning {@code null} if skyframe deps were missing and have been requested. + * Compute the StarlarkImportLookupValue for all given keys using vanilla Skyframe evaluation, + * returning {@code null} if Skyframe deps were missing and have been requested. */ @Nullable private static List<StarlarkImportLookupValue> computeStarlarkImportsNoInlining( @@ -668,9 +668,9 @@ } /** - * Compute the StarlarkImportLookupValue for all given SkyKeys by "inlining" the - * StarlarkImportLookupFunction and bypassing traditional skyframe evaluation, returning {@code - * null} if skyframe deps were missing and have been requested. + * Compute the StarlarkImportLookupValue for all given keys by "inlining" the + * StarlarkImportLookupFunction and bypassing traditional Skyframe evaluation, returning {@code + * null} if Skyframe deps were missing and have been requested. */ @Nullable private static List<StarlarkImportLookupValue> computeStarlarkImportsWithInlining( @@ -682,20 +682,19 @@ Lists.newArrayListWithExpectedSize(keys.size()); Exception deferredException = null; boolean valuesMissing = false; - // For each listed import in order, try to compute its StarlarkImportLookupValue. + // Compute StarlarkImportLookupValue 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.) Map<StarlarkImportLookupValue.Key, CachedStarlarkImportLookupValueAndDeps> visitedDepsInToplevelLoad = new HashMap<>(); for (StarlarkImportLookupValue.Key key : keys) { SkyValue skyValue; try { - if (visitedDepsInToplevelLoad.containsKey(key)) { - skyValue = visitedDepsInToplevelLoad.get(key).getValue(); - } else { - skyValue = - starlarkImportLookupFunctionForInlining - .computeWithSelfInlineCallsForPackageAndWorkspaceNodes( - key, env, visitedDepsInToplevelLoad); - } + // Will complete right away if it's already cached in visitedDepsInToplevelLoad. + skyValue = + starlarkImportLookupFunctionForInlining + .computeWithSelfInlineCallsForPackageAndWorkspaceNodes( + key, env, visitedDepsInToplevelLoad); } catch (StarlarkImportFailedException | InconsistentFilesystemException e) { // For determinism's sake while inlining, preserve the first exception and continue to run // subsequently listed imports to completion/exception, loading all transitive deps anyway.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java index cb92629..cb27011 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java
@@ -172,7 +172,7 @@ @Nullable StarlarkImportLookupValue computeWithSelfInlineCallsForPackageAndWorkspaceNodes( - SkyKey skyKey, + StarlarkImportLookupValue.Key key, Environment env, Map<StarlarkImportLookupValue.Key, CachedStarlarkImportLookupValueAndDeps> visitedDepsInToplevelLoad) @@ -185,7 +185,7 @@ // error. CachedStarlarkImportLookupValueAndDeps cachedStarlarkImportLookupValueAndDeps = computeWithSelfInlineCallsInternal( - skyKey, + key, env, /*visitedNested=*/ new LinkedHashSet<>(), /*visitedDepsInToplevelLoad=*/ visitedDepsInToplevelLoad); @@ -197,27 +197,39 @@ @Nullable private CachedStarlarkImportLookupValueAndDeps computeWithSelfInlineCallsInternal( - SkyKey skyKey, + StarlarkImportLookupValue.Key key, Environment env, Set<Label> visitedNested, Map<StarlarkImportLookupValue.Key, CachedStarlarkImportLookupValueAndDeps> visitedDepsInToplevelLoad) throws InconsistentFilesystemException, StarlarkImportFailedException, InterruptedException { - StarlarkImportLookupValue.Key key = (StarlarkImportLookupValue.Key) skyKey.argument(); - Label label = key.getLabel(); - - // If we've visited a StarlarkImportLookupValue through some other load path for a given - // package, we must use the existing value to preserve reference equality between Starlark - // values that ought to be the same. See b/138598337 for details. + // Under StarlarkImportLookupFunction 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). + // + // 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 + // WORKSPACE file has a diamond dependency on foo.bzl, evaluates it the first time, and gets a + // different copy of it from the cache the second time. This is because Starlark values may use + // object identity, which breaks the moment two distinct observable copies are visible in the + // same context (see b/138598337). + // + // (Note that blocking evaluation of .bzls on retrievals from the global cache doesn't work -- + // 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.) + // CachedStarlarkImportLookupValueAndDeps cachedStarlarkImportLookupValueAndDeps = visitedDepsInToplevelLoad.get(key); if (cachedStarlarkImportLookupValueAndDeps == null) { - // Note that we can't block other threads on the computation of this value due to a potential - // deadlock on a cycle. Although we are repeating some work, it is possible we have an import - // cycle where one thread starts at one side of the cycle and the other thread starts at the - // other side, and they then wait forever on the results of each others computations. cachedStarlarkImportLookupValueAndDeps = - selfInliningManager.starlarkImportLookupValueCache.getIfPresent(skyKey); + selfInliningManager.starlarkImportLookupValueCache.getIfPresent(key); if (cachedStarlarkImportLookupValueAndDeps != null) { cachedStarlarkImportLookupValueAndDeps.traverse( env::registerDependencies, visitedDepsInToplevelLoad); @@ -227,6 +239,7 @@ return cachedStarlarkImportLookupValueAndDeps; } + Label label = key.getLabel(); if (!visitedNested.add(label)) { ImmutableList<Label> cycle = CycleUtils.splitIntoPathAndChain(Predicates.equalTo(label), visitedNested).second; @@ -263,7 +276,7 @@ cachedStarlarkImportLookupValueAndDeps = inlineCachedValueBuilder.build(); visitedDepsInToplevelLoad.put(key, cachedStarlarkImportLookupValueAndDeps); selfInliningManager.starlarkImportLookupValueCache.put( - skyKey, cachedStarlarkImportLookupValueAndDeps); + key, cachedStarlarkImportLookupValueAndDeps); } return cachedStarlarkImportLookupValueAndDeps; } @@ -329,9 +342,9 @@ // to handle though. @Nullable private StarlarkImportLookupValue computeInternal( - SkyKey skyKey, Environment env, @Nullable InliningState inliningState) + StarlarkImportLookupValue.Key key, Environment env, @Nullable InliningState inliningState) throws InconsistentFilesystemException, StarlarkImportFailedException, InterruptedException { - Label label = ((StarlarkImportLookupValue.Key) skyKey).getLabel(); + Label label = key.getLabel(); PathFragment filePath = label.toPathFragment(); StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); @@ -358,7 +371,7 @@ try { result = computeInternalWithAst( - skyKey, filePath, starlarkSemantics, astLookupValue, env, inliningState); + key, filePath, starlarkSemantics, astLookupValue, env, inliningState); } catch (InconsistentFilesystemException | StarlarkImportFailedException | InterruptedException e) { @@ -374,14 +387,13 @@ @Nullable private StarlarkImportLookupValue computeInternalWithAst( - SkyKey skyKey, + StarlarkImportLookupValue.Key key, PathFragment filePath, StarlarkSemantics starlarkSemantics, ASTFileLookupValue astLookupValue, Environment env, @Nullable InliningState inliningState) throws InconsistentFilesystemException, StarlarkImportFailedException, InterruptedException { - StarlarkImportLookupValue.Key key = (StarlarkImportLookupValue.Key) skyKey; Label label = key.getLabel(); if (!astLookupValue.lookupSuccessful()) { @@ -458,8 +470,7 @@ } private static ImmutableMap<RepositoryName, RepositoryName> getRepositoryMapping( - SkyKey skyKey, Environment env) throws InterruptedException { - StarlarkImportLookupValue.Key key = (StarlarkImportLookupValue.Key) skyKey; + StarlarkImportLookupValue.Key key, Environment env) throws InterruptedException { Label enclosingFileLabel = key.getLabel(); ImmutableMap<RepositoryName, RepositoryName> repositoryMapping; @@ -556,19 +567,19 @@ } /** - * Compute the StarlarkImportLookupValue for all given SkyKeys using vanilla skyframe evaluation, - * returning {@code null} if skyframe deps were missing and have been requested. + * Compute the StarlarkImportLookupValue for all given keys using vanilla Skyframe evaluation, + * returning {@code null} if Skyframe deps were missing and have been requested. */ @Nullable private static List<StarlarkImportLookupValue> computeStarlarkImportsNoInlining( - Environment env, List<? extends SkyKey> keys, Location locationForErrors) + Environment env, List<StarlarkImportLookupValue.Key> keys, Location locationForErrors) throws StarlarkImportFailedException, InterruptedException { List<StarlarkImportLookupValue> starlarkImports = Lists.newArrayListWithExpectedSize(keys.size()); Map<SkyKey, ValueOrException<StarlarkImportFailedException>> values = env.getValuesOrThrow(keys, StarlarkImportFailedException.class); // Uses same order as load()s in the file. Order matters since we report the first error. - for (SkyKey key : keys) { + for (StarlarkImportLookupValue.Key key : keys) { try { starlarkImports.add((StarlarkImportLookupValue) values.get(key).get()); } catch (StarlarkImportFailedException exn) { @@ -580,13 +591,16 @@ } /** - * Compute the StarlarkImportLookupValue for all given SkyKeys by reusing this instance of the - * StarlarkImportLookupFunction, bypassing traditional skyframe evaluation, returning {@code null} - * if skyframe deps were missing and have been requested. + * Compute the StarlarkImportLookupValue for all given keys by reusing this instance of the + * StarlarkImportLookupFunction, bypassing traditional Skyframe evaluation, returning {@code null} + * if Skyframe deps were missing and have been requested. */ @Nullable private List<StarlarkImportLookupValue> computeStarlarkImportsWithSelfInlining( - Environment env, List<? extends SkyKey> keys, Label fileLabel, InliningState inliningState) + Environment env, + List<StarlarkImportLookupValue.Key> keys, + Label fileLabel, + InliningState inliningState) throws InterruptedException, StarlarkImportFailedException, InconsistentFilesystemException { Preconditions.checkState( env instanceof RecordingSkyFunctionEnvironment, @@ -598,7 +612,7 @@ Exception deferredException = null; boolean valuesMissing = false; // NOTE: Iterating over imports in the order listed in the file. - for (SkyKey key : keys) { + for (StarlarkImportLookupValue.Key key : keys) { CachedStarlarkImportLookupValueAndDeps cachedValue; try { cachedValue = @@ -839,7 +853,8 @@ private static class SelfInliningManager { private final int starlarkImportLookupValueCacheSize; - private Cache<SkyKey, CachedStarlarkImportLookupValueAndDeps> starlarkImportLookupValueCache; + private Cache<StarlarkImportLookupValue.Key, CachedStarlarkImportLookupValueAndDeps> + starlarkImportLookupValueCache; private CachedStarlarkImportLookupValueAndDepsBuilderFactory cachedStarlarkImportLookupValueAndDepsBuilderFactory = new CachedStarlarkImportLookupValueAndDepsBuilderFactory();