Fix PackageFunction's call to Package.Builder.Helper#onLoadingComplete to pass
along the wall time of the load, even when the package in question was in PackageFunction's
internal cache (e.g. the current #compute call is a PackageFunction Skyframe restart).
Also clarify the intent of the 'loadTimeMs' param in #onLoadingComplete.
RELNOTES: None
PiperOrigin-RevId: 188253198
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index 9db519e..03f2de6 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -732,7 +732,10 @@
*
* @param pkg the loaded {@link Package}
* @param skylarkSemantics are the semantics used to load the package
- * @param loadTimeMs the wall time, in ms, that it took to load the package
+ * @param loadTimeMs the wall time, in ms, that it took to load the package. More precisely,
+ * this is the wall time of the call to {@link PackageFactory#createPackageFromAst}.
+ * Notably, this does not include the time to read and parse the package's BUILD file, nor
+ * the time to read, parse, or evaluate any of the transitively loaded .bzl files.
*/
void onLoadingComplete(Package pkg, SkylarkSemantics skylarkSemantics, long loadTimeMs);
}
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 bfc2a35..b7bd7de 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
@@ -94,7 +94,7 @@
private final PackageFactory packageFactory;
private final CachingPackageLocator packageLocator;
- private final Cache<PackageIdentifier, BuilderAndGlobDeps> packageFunctionCache;
+ private final Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache;
private final Cache<PackageIdentifier, AstParseResult> astCache;
private final AtomicBoolean showLoadingProgress;
private final AtomicInteger numPackagesLoaded;
@@ -115,7 +115,7 @@
PackageFactory packageFactory,
CachingPackageLocator pkgLocator,
AtomicBoolean showLoadingProgress,
- Cache<PackageIdentifier, BuilderAndGlobDeps> packageFunctionCache,
+ Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache,
Cache<PackageIdentifier, AstParseResult> astCache,
AtomicInteger numPackagesLoaded,
@Nullable SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining,
@@ -143,7 +143,7 @@
PackageFactory packageFactory,
CachingPackageLocator pkgLocator,
AtomicBoolean showLoadingProgress,
- Cache<PackageIdentifier, BuilderAndGlobDeps> packageFunctionCache,
+ Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache,
Cache<PackageIdentifier, AstParseResult> astCache,
AtomicInteger numPackagesLoaded,
@Nullable SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining) {
@@ -207,13 +207,16 @@
}
/** An entry in {@link PackageFunction} internal cache. */
- public static class BuilderAndGlobDeps {
+ public static class LoadedPackageCacheEntry {
private final Package.Builder builder;
private final Set<SkyKey> globDepKeys;
+ private final long loadTimeNanos;
- private BuilderAndGlobDeps(Package.Builder builder, Set<SkyKey> globDepKeys) {
+ private LoadedPackageCacheEntry(
+ Package.Builder builder, Set<SkyKey> globDepKeys, long loadTimeNanos) {
this.builder = builder;
this.globDepKeys = globDepKeys;
+ this.loadTimeNanos = loadTimeNanos;
}
}
@@ -581,8 +584,7 @@
List<Statement> preludeStatements =
astLookupValue.lookupSuccessful()
? astLookupValue.getAST().getStatements() : ImmutableList.<Statement>of();
- long startTimeNanos = BlazeClock.nanoTime();
- BuilderAndGlobDeps packageBuilderAndGlobDeps =
+ LoadedPackageCacheEntry packageCacheEntry =
loadPackage(
workspaceName,
replacementContents,
@@ -594,11 +596,10 @@
preludeStatements,
packageLookupValue.getRoot(),
env);
- long loadTimeNanos = Math.max(BlazeClock.nanoTime() - startTimeNanos, 0L);
- if (packageBuilderAndGlobDeps == null) {
+ if (packageCacheEntry == null) {
return null;
}
- Package.Builder pkgBuilder = packageBuilderAndGlobDeps.builder;
+ Package.Builder pkgBuilder = packageCacheEntry.builder;
try {
pkgBuilder.buildPartial();
} catch (NoSuchPackageException e) {
@@ -619,7 +620,7 @@
e.toNoSuchPackageException(),
e.isTransient() ? Transience.TRANSIENT : Transience.PERSISTENT);
}
- Set<SkyKey> globKeys = packageBuilderAndGlobDeps.globDepKeys;
+ Set<SkyKey> globKeys = packageCacheEntry.globDepKeys;
Map<Label, Path> subincludes = pkgBuilder.getSubincludes();
boolean packageShouldBeConsideredInError;
try {
@@ -649,7 +650,7 @@
// We know this SkyFunction will not be called again, so we can remove the cache entry.
packageFunctionCache.invalidate(packageId);
- packageFactory.afterDoneLoadingPackage(pkg, skylarkSemantics, loadTimeNanos);
+ packageFactory.afterDoneLoadingPackage(pkg, skylarkSemantics, packageCacheEntry.loadTimeNanos);
return new PackageValue(pkg);
}
@@ -1279,7 +1280,7 @@
* latter indicates that we have a legitimate BUILD file and should actually read its contents.
*/
@Nullable
- private BuilderAndGlobDeps loadPackage(
+ private LoadedPackageCacheEntry loadPackage(
String workspaceName,
@Nullable String replacementContents,
PackageIdentifier packageId,
@@ -1291,8 +1292,8 @@
Root packageRoot,
Environment env)
throws InterruptedException, PackageFunctionException {
- BuilderAndGlobDeps builderAndGlobDeps = packageFunctionCache.getIfPresent(packageId);
- if (builderAndGlobDeps == null) {
+ LoadedPackageCacheEntry packageCacheEntry = packageFunctionCache.getIfPresent(packageId);
+ if (packageCacheEntry == null) {
profiler.startTask(ProfilerTask.CREATE_PACKAGE, packageId.toString());
if (packageProgress != null) {
packageProgress.startReadPackage(packageId);
@@ -1360,6 +1361,7 @@
astCache.invalidate(packageId);
GlobberWithSkyframeGlobDeps globberWithSkyframeGlobDeps =
makeGlobber(buildFilePath, packageId, packageRoot, env);
+ long startTimeNanos = BlazeClock.nanoTime();
Package.Builder pkgBuilder = packageFactory.createPackageFromAst(
workspaceName,
packageId,
@@ -1370,18 +1372,21 @@
defaultVisibility,
skylarkSemantics,
globberWithSkyframeGlobDeps);
- builderAndGlobDeps =
- new BuilderAndGlobDeps(pkgBuilder, globberWithSkyframeGlobDeps.getGlobDepsRequested());
+ long loadTimeNanos = Math.max(BlazeClock.nanoTime() - startTimeNanos, 0L);
+ packageCacheEntry = new LoadedPackageCacheEntry(
+ pkgBuilder,
+ globberWithSkyframeGlobDeps.getGlobDepsRequested(),
+ loadTimeNanos);
numPackagesLoaded.incrementAndGet();
if (packageProgress != null) {
packageProgress.doneReadPackage(packageId);
}
- packageFunctionCache.put(packageId, builderAndGlobDeps);
+ packageFunctionCache.put(packageId, packageCacheEntry);
} finally {
profiler.completeTask(ProfilerTask.CREATE_PACKAGE);
}
}
- return builderAndGlobDeps;
+ return packageCacheEntry;
}
private static class InternalInconsistentFilesystemException extends Exception {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 41106fc..8390a34 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -122,6 +122,7 @@
import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
import com.google.devtools.build.lib.skyframe.PackageFunction.IncrementalityIntent;
+import com.google.devtools.build.lib.skyframe.PackageFunction.LoadedPackageCacheEntry;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ActionCompletedReceiver;
import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ProgressSupplier;
@@ -215,7 +216,7 @@
// dependencies).
// TODO(bazel-team): remove this cache once we have skyframe-native package loading
// [skyframe-loading]
- private final Cache<PackageIdentifier, PackageFunction.BuilderAndGlobDeps>
+ private final Cache<PackageIdentifier, LoadedPackageCacheEntry>
packageFunctionCache = newPkgFunctionCache();
private final Cache<PackageIdentifier, AstParseResult> astCache = newAstCache();
@@ -485,7 +486,7 @@
PackageFactory pkgFactory,
PackageManager packageManager,
AtomicBoolean showLoadingProgress,
- Cache<PackageIdentifier, PackageFunction.BuilderAndGlobDeps> packageFunctionCache,
+ Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache,
Cache<PackageIdentifier, AstParseResult> astCache,
AtomicInteger numPackagesLoaded,
RuleClassProvider ruleClassProvider,
@@ -803,7 +804,7 @@
}
}
- protected Cache<PackageIdentifier, PackageFunction.BuilderAndGlobDeps>
+ protected Cache<PackageIdentifier, LoadedPackageCacheEntry>
newPkgFunctionCache() {
return CacheBuilder.newBuilder().build();
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
index 5613c87..fa5bbd6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
@@ -54,6 +54,7 @@
import com.google.devtools.build.lib.skyframe.PackageFunction;
import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
import com.google.devtools.build.lib.skyframe.PackageFunction.IncrementalityIntent;
+import com.google.devtools.build.lib.skyframe.PackageFunction.LoadedPackageCacheEntry;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
import com.google.devtools.build.lib.skyframe.PackageValue;
@@ -341,7 +342,7 @@
protected final ImmutableMap<SkyFunctionName, SkyFunction> makeFreshSkyFunctions() {
AtomicReference<TimestampGranularityMonitor> tsgm =
new AtomicReference<>(new TimestampGranularityMonitor(BlazeClock.instance()));
- Cache<PackageIdentifier, PackageFunction.BuilderAndGlobDeps> packageFunctionCache =
+ Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache =
CacheBuilder.newBuilder().build();
Cache<PackageIdentifier, AstParseResult> astCache = CacheBuilder.newBuilder().build();
AtomicReference<PerBuildSyscallCache> syscallCacheRef = new AtomicReference<>(
