SourceArtifacts are interned on deserialization using an ArtifactFactory. This should reduce memory consumption in NestedSet deserialization, which currently does not recycle Artifact instances.

PiperOrigin-RevId: 194083901

8 files changed, 217 insertions, 11 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index b77ca28199..84110fe84f 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -27,9 +27,14 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
+import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.shell.ShellUtils;
+import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext;
+import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
+import com.google.devtools.build.lib.skyframe.serialization.SerializationContext;
+import com.google.devtools.build.lib.skyframe.serialization.SerializationException;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
@@ -43,6 +48,9 @@ import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.protobuf.CodedInputStream;
+import com.google.protobuf.CodedOutputStream;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Comparator;
@@ -468,10 +476,9 @@ public class Artifact
    *
    * TODO(shahan): move {@link Artifact#getPath} to this subclass.
    * */
-  @AutoCodec
   public static final class SourceArtifact extends Artifact {
-    @AutoCodec.VisibleForSerialization
-    SourceArtifact(ArtifactRoot root, PathFragment execPath, ArtifactOwner owner) {
+    @VisibleForTesting
+    public SourceArtifact(ArtifactRoot root, PathFragment execPath, ArtifactOwner owner) {
       super(root, execPath, owner);
     }
 
@@ -742,6 +749,37 @@ public class Artifact
     }
   }
 
+  /** {@link ObjectCodec} for {@link SourceArtifact} */
+  private static class SourceArtifactCodec implements ObjectCodec<SourceArtifact> {
+
+    @Override
+    public Class<? extends SourceArtifact> getEncodedClass() {
+      return SourceArtifact.class;
+    }
+
+    @Override
+    public void serialize(
+        SerializationContext context, SourceArtifact obj, CodedOutputStream codedOut)
+        throws SerializationException, IOException {
+      context.serialize(obj.getExecPath(), codedOut);
+      context.serialize(obj.getRoot(), codedOut);
+      context.serialize(obj.getArtifactOwner(), codedOut);
+    }
+
+    @Override
+    public SourceArtifact deserialize(DeserializationContext context, CodedInputStream codedIn)
+        throws SerializationException, IOException {
+      PathFragment execPath = context.deserialize(codedIn);
+      ArtifactRoot artifactRoot = context.deserialize(codedIn);
+      ArtifactOwner owner = context.deserialize(codedIn);
+      return (SourceArtifact)
+          context
+              .getDependency(ArtifactResolverSupplier.class)
+              .get()
+              .getSourceArtifact(execPath, artifactRoot.getRoot(), owner);
+    }
+  }
+
   // ---------------------------------------------------------------------------
   // Static methods to assist in working with Artifacts
 
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
index f697401b60..25af4fe5f7 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.actions;
 
+import com.google.common.base.Supplier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -80,4 +81,10 @@ public interface ArtifactResolver {
       Iterable<PathFragment> execPaths, PackageRootResolver resolver) throws InterruptedException;
 
   Path getPathFromSourceExecPath(PathFragment execPath);
+
+  /**
+   * Supplies an {@link ArtifactFactory}. We define a custom interface because parameterized types
+   * are not allowed as dependencies to serialization.
+   */
+  interface ArtifactResolverSupplier extends Supplier<ArtifactResolver> {}
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index 937be885d3..c02ca2cb1a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -150,7 +150,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
       CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
       List<BuildFileName> buildFilesByPriority,
       ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile,
-      BuildOptions defaultBuildOptions) {
+      BuildOptions defaultBuildOptions,
+      MutableArtifactFactorySupplier mutableArtifactFactorySupplier) {
     super(
         evaluatorSupplier,
         pkgFactory,
@@ -168,7 +169,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
         actionOnIOExceptionReadingBuildFile,
         /*shouldUnblockCpuWorkWhenFetchingDeps=*/ false,
         defaultBuildOptions,
-        new PackageProgressReceiver());
+        new PackageProgressReceiver(),
+        mutableArtifactFactorySupplier);
     this.diffAwarenessManager = new DiffAwarenessManager(diffAwarenessFactories);
     this.customDirtinessCheckers = customDirtinessCheckers;
   }
@@ -189,6 +191,42 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
       List<BuildFileName> buildFilesByPriority,
       ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile,
       BuildOptions defaultBuildOptions) {
+    return create(
+        pkgFactory,
+        fileSystem,
+        directories,
+        actionKeyContext,
+        workspaceStatusActionFactory,
+        buildInfoFactories,
+        diffAwarenessFactories,
+        extraSkyFunctions,
+        customDirtinessCheckers,
+        hardcodedBlacklistedPackagePrefixes,
+        additionalBlacklistedPackagePrefixesFile,
+        crossRepositoryLabelViolationStrategy,
+        buildFilesByPriority,
+        actionOnIOExceptionReadingBuildFile,
+        defaultBuildOptions,
+        new MutableArtifactFactorySupplier());
+  }
+
+  public static SequencedSkyframeExecutor create(
+      PackageFactory pkgFactory,
+      FileSystem fileSystem,
+      BlazeDirectories directories,
+      ActionKeyContext actionKeyContext,
+      Factory workspaceStatusActionFactory,
+      ImmutableList<BuildInfoFactory> buildInfoFactories,
+      Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories,
+      ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions,
+      Iterable<SkyValueDirtinessChecker> customDirtinessCheckers,
+      ImmutableSet<PathFragment> hardcodedBlacklistedPackagePrefixes,
+      PathFragment additionalBlacklistedPackagePrefixesFile,
+      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
+      List<BuildFileName> buildFilesByPriority,
+      ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile,
+      BuildOptions defaultBuildOptions,
+      MutableArtifactFactorySupplier mutableArtifactFactorySupplier) {
     SequencedSkyframeExecutor skyframeExecutor =
         new SequencedSkyframeExecutor(
             InMemoryMemoizingEvaluator.SUPPLIER,
@@ -206,7 +244,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
             crossRepositoryLabelViolationStrategy,
             buildFilesByPriority,
             actionOnIOExceptionReadingBuildFile,
-            defaultBuildOptions);
+            defaultBuildOptions,
+            mutableArtifactFactorySupplier);
     skyframeExecutor.init();
     return skyframeExecutor;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java
index ddb1a7c09d..27f83013d3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.packages.PackageFactory;
+import com.google.devtools.build.lib.skyframe.SkyframeExecutor.MutableArtifactFactorySupplier;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -62,6 +63,7 @@ public class SequencedSkyframeExecutorFactory implements SkyframeExecutorFactory
         BazelSkyframeExecutorConstants.CROSS_REPOSITORY_LABEL_VIOLATION_STRATEGY,
         BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY,
         BazelSkyframeExecutorConstants.ACTION_ON_IO_EXCEPTION_READING_BUILD_FILE,
-        defaultBuildOptions);
+        defaultBuildOptions,
+        new MutableArtifactFactorySupplier());
   }
 }
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 d152562513..e0fc14bf5c 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
@@ -49,6 +49,7 @@ import com.google.devtools.build.lib.actions.ActionLookupValue;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
+import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
@@ -251,7 +252,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   // Under normal circumstances, the artifact factory persists for the life of a Blaze server, but
   // since it is not yet created when we create the value builders, we have to use a supplier,
   // initialized when the build view is created.
-  private final MutableSupplier<ArtifactFactory> artifactFactory = new MutableSupplier<>();
+  private final MutableArtifactFactorySupplier artifactFactory;
   // Used to give to WriteBuildInfoAction via a supplier. Relying on BuildVariableValue.BUILD_ID
   // would be preferable, but we have no way to have the Action depend on that value directly.
   // Having the BuildInfoFunction own the supplier is currently not possible either, because then
@@ -307,6 +308,21 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
   private static final Logger logger = Logger.getLogger(SkyframeExecutor.class.getName());
 
+  /** An {@link ArtifactResolverSupplier} that supports setting of an {@link ArtifactFactory}. */
+  public static class MutableArtifactFactorySupplier implements ArtifactResolverSupplier {
+
+    private ArtifactFactory artifactFactory;
+
+    void set(ArtifactFactory artifactFactory) {
+      this.artifactFactory = artifactFactory;
+    }
+
+    @Override
+    public ArtifactFactory get() {
+      return artifactFactory;
+    }
+  }
+
   protected SkyframeExecutor(
       EvaluatorSupplier evaluatorSupplier,
       PackageFactory pkgFactory,
@@ -324,7 +340,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile,
       boolean shouldUnblockCpuWorkWhenFetchingDeps,
       BuildOptions defaultBuildOptions,
-      @Nullable PackageProgressReceiver packageProgress) {
+      @Nullable PackageProgressReceiver packageProgress,
+      MutableArtifactFactorySupplier artifactResolverSupplier) {
     // Strictly speaking, these arguments are not required for initialization, but all current
     // callsites have them at hand, so we might as well set them during construction.
     this.evaluatorSupplier = evaluatorSupplier;
@@ -356,6 +373,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         directories,
         this,
         (ConfiguredRuleClassProvider) ruleClassProvider);
+    this.artifactFactory = artifactResolverSupplier;
     this.artifactFactory.set(skyframeBuildView.getArtifactFactory());
     this.externalFilesHelper = ExternalFilesHelper.create(
         pkgLocator, this.externalFileAction, directories);
@@ -481,7 +499,10 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     map.put(
         SkyFunctions.BUILD_INFO_COLLECTION,
         new BuildInfoCollectionFunction(
-            actionKeyContext, artifactFactory, buildInfoFactories, removeActionsAfterEvaluation));
+            actionKeyContext,
+            artifactFactory::get,
+            buildInfoFactories,
+            removeActionsAfterEvaluation));
     map.put(
         SkyFunctions.BUILD_INFO,
         new WorkspaceStatusFunction(removeActionsAfterEvaluation, this::makeWorkspaceStatusAction));
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/BUILD
index 02fa5f90f8..c57f70010f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/BUILD
@@ -10,7 +10,10 @@ java_library(
     testonly = 1,
     srcs = glob(
         ["*.java"],
-        exclude = ["FakeDirectories.java"],
+        exclude = [
+            "FakeDirectories.java",
+            "SerializationDepsUtils.java",
+        ],
     ),
     deps = [
         "//src/main/java/com/google/devtools/build/lib:syntax",
@@ -27,6 +30,19 @@ java_library(
 )
 
 java_library(
+    name = "depsutils",
+    srcs = ["SerializationDepsUtils.java"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib/actions",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//third_party:guava",
+        "//third_party:jsr305",
+    ],
+)
+
+java_library(
     name = "fake_directories",
     testonly = 1,
     srcs = ["FakeDirectories.java"],
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationDepsUtils.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationDepsUtils.java
new file mode 100644
index 0000000000..f80b78d8b9
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationDepsUtils.java
@@ -0,0 +1,77 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe.serialization.testutils;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
+import com.google.devtools.build.lib.actions.ArtifactOwner;
+import com.google.devtools.build.lib.actions.ArtifactResolver;
+import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
+import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.actions.PackageRootResolver;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
+import java.util.Map;
+import javax.annotation.Nullable;
+
+/** Utilities for testing with serialization dependencies. */
+public class SerializationDepsUtils {
+
+  /** Default serialization dependencies for testing. */
+  public static final ImmutableMap<Class<?>, Object> SERIALIZATION_DEPS_FOR_TEST =
+      ImmutableMap.of(ArtifactResolverSupplier.class, new ArtifactResolverSupplierForTest());
+
+  /**
+   * An {@link ArtifactResolverSupplier} that calls directly into the {@link SourceArtifact}
+   * constructor.
+   */
+  public static class ArtifactResolverSupplierForTest implements ArtifactResolverSupplier {
+
+    @Override
+    public ArtifactResolver get() {
+      return new ArtifactResolver() {
+        @Override
+        public Artifact getSourceArtifact(PathFragment execPath, Root root, ArtifactOwner owner) {
+          return new SourceArtifact(ArtifactRoot.asSourceRoot(root), execPath, owner);
+        }
+
+        @Override
+        public Artifact getSourceArtifact(PathFragment execPath, Root root) {
+          throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public Artifact resolveSourceArtifact(
+            PathFragment execPath, RepositoryName repositoryName) {
+          throw new UnsupportedOperationException();
+        }
+
+        @Nullable
+        @Override
+        public Map<PathFragment, Artifact> resolveSourceArtifacts(
+            Iterable<PathFragment> execPaths, PackageRootResolver resolver) {
+          throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public Path getPathFromSourceExecPath(PathFragment execPath) {
+          throw new UnsupportedOperationException();
+        }
+      };
+    }
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationTester.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationTester.java
index c10143340f..ff4dbe8b4d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationTester.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationTester.java
@@ -30,6 +30,7 @@ import com.google.protobuf.ByteString;
 import com.google.protobuf.InvalidProtocolBufferException;
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Map;
 import java.util.Random;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -84,6 +85,11 @@ public class SerializationTester {
     return this;
   }
 
+  public SerializationTester addDependencies(Map<Class<?>, Object> dependencies) {
+    dependenciesBuilder.putAll(dependencies);
+    return this;
+  }
+
   public SerializationTester addCodec(ObjectCodec<?> codec) {
     additionalCodecs.add(codec);
     return this;