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

4 files changed, 34 insertions, 24 deletions

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 9db519ed26..03f2de60e0 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 @@ public class Package {
        *
        * @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 bfc2a35e72..b7bd7de706 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 @@ public class PackageFunction implements SkyFunction {
 
   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 @@ public class PackageFunction implements SkyFunction {
       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 @@ public class PackageFunction implements SkyFunction {
       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 @@ public class PackageFunction implements SkyFunction {
   }
 
   /** 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 @@ public class PackageFunction implements SkyFunction {
     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 @@ public class PackageFunction implements SkyFunction {
             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 @@ public class PackageFunction implements SkyFunction {
           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 @@ public class PackageFunction implements SkyFunction {
     // 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 @@ public class PackageFunction implements SkyFunction {
    * 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 @@ public class PackageFunction implements SkyFunction {
       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 @@ public class PackageFunction implements SkyFunction {
         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 @@ public class PackageFunction implements SkyFunction {
             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 41106fca89..8390a34d9a 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.DirtinessCheckerUtils.FileDirtines
 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   // 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     }
   }
 
-  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 5613c87ab1..fa5bbd66dd 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.FileSymlinkInfiniteExpansionUnique
 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 @@ public abstract class AbstractPackageLoader implements PackageLoader {
   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<>(