Use INTERNER to manage canonical instances of source artifact roots.

Blaze benchmark runs show there is no meaningful heap memory usage difference between this and the current solution, which relies on a predefined map.

In contrast, when the Interner calls are removed from this CL, Blaze benchmark consistently shows around 7MB of heap memory usage increase, which is precisely as expected.

PiperOrigin-RevId: 339959603

src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java

@@ -15,9 +15,9 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
+import com.google.devtools.build.lib.cmdline.LabelConstants;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
@@ -37,8 +37,8 @@
 public class ArtifactFactory implements ArtifactResolver {
 
   private final Path execRootParent;
+  private final Path externalSourceBase;
   private final PathFragment derivedPathPrefix;
-  private ImmutableMap<Root, ArtifactRoot> sourceArtifactRoots;
   private boolean siblingRepositoryLayout = false;
 
   /**
@@ -123,6 +123,10 @@ void clear() {
    */
   public ArtifactFactory(Path execRootParent, String derivedPathPrefix) {
     this.execRootParent = execRootParent;
+    this.externalSourceBase =
+        execRootParent
+            .getParentDirectory()
+            .getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION);
     this.derivedPathPrefix = PathFragment.create(derivedPathPrefix);
   }
 
@@ -134,11 +138,6 @@ public synchronized void clear() {
     sourceArtifactCache.clear();
   }
 
-  public synchronized void setSourceArtifactRoots(
-      ImmutableMap<Root, ArtifactRoot> sourceArtifactRoots) {
-    this.sourceArtifactRoots = sourceArtifactRoots;
-  }
-
   public void setSiblingRepositoryLayout(boolean siblingRepositoryLayout) {
     this.siblingRepositoryLayout = siblingRepositoryLayout;
   }
@@ -162,20 +161,13 @@ public SourceArtifact getSourceArtifact(PathFragment execPath, Root root, Artifa
     Preconditions.checkArgument(
         execPath.isAbsolute() == root.isAbsolute(), "%s %s %s", execPath, root, owner);
     Preconditions.checkNotNull(owner, "%s %s", execPath, root);
-    Preconditions.checkNotNull(
-        sourceArtifactRoots, "Not initialized for %s %s %s", execPath, root, owner);
+    // TODO(jungjw): Come up with a more reliable way to distinguish external source roots.
+    ArtifactRoot artifactRoot =
+        root.asPath() != null && root.asPath().startsWith(externalSourceBase)
+            ? ArtifactRoot.asExternalSourceRoot(root)
+            : ArtifactRoot.asSourceRoot(root);
     return (SourceArtifact)
-        getArtifact(
-            Preconditions.checkNotNull(
-                sourceArtifactRoots.get(root),
-                "%s has no ArtifactRoot (%s) in %s",
-                root,
-                execPath,
-                sourceArtifactRoots),
-            execPath,
-            owner,
-            null,
-            /*contentBasedPath=*/ false);
+        getArtifact(artifactRoot, execPath, owner, null, /*contentBasedPath=*/ false);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/actions/ArtifactRoot.java

@@ -56,7 +56,7 @@ public final class ArtifactRoot implements Comparable<ArtifactRoot>, Serializabl
    * <p>Returns the given path as a source root. The path may not be {@code null}.
    */
   public static ArtifactRoot asSourceRoot(Root root) {
-    return new ArtifactRoot(root, PathFragment.EMPTY_FRAGMENT, RootType.Source);
+    return INTERNER.intern(new ArtifactRoot(root, PathFragment.EMPTY_FRAGMENT, RootType.Source));
   }
 
   /**
@@ -70,7 +70,8 @@ public static ArtifactRoot asSourceRoot(Root root) {
   public static ArtifactRoot asExternalSourceRoot(Root root) {
     Preconditions.checkArgument(
         root.asPath().asFragment().endsWith(LabelConstants.EXTERNAL_REPOSITORY_LOCATION));
-    return new ArtifactRoot(root, PathFragment.EMPTY_FRAGMENT, RootType.ExternalSource);
+    return INTERNER.intern(
+        new ArtifactRoot(root, PathFragment.EMPTY_FRAGMENT, RootType.ExternalSource));
   }
 
   /**

src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java

@@ -26,7 +26,6 @@
 import com.google.common.collect.Sets;
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
-import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.FileStateType;
 import com.google.devtools.build.lib.actions.FileStateValue;
@@ -302,10 +301,8 @@ private static boolean nestedSetAsSkyKeyOptionsChanged(OptionsProvider options)
           SkyFunctions.TARGET_PATTERN_PHASE);
 
   @Override
-  protected ImmutableMap<Root, ArtifactRoot> createSourceArtifactRootMapOnNewPkgLocator(
-      PathPackageLocator oldLocator, PathPackageLocator pkgLocator) {
+  protected void onPkgLocatorChange(PathPackageLocator oldLocator, PathPackageLocator pkgLocator) {
     invalidate(SkyFunctionName.functionIsIn(PACKAGE_LOCATOR_DEPENDENT_VALUES));
-    return super.createSourceArtifactRootMapOnNewPkgLocator(oldLocator, pkgLocator);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -101,7 +101,6 @@
 import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition.TransitionException;
 import com.google.devtools.build.lib.buildtool.BuildRequestOptions;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelConstants;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
@@ -223,7 +222,6 @@
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.StarlarkSemantics;
 import net.starlark.java.syntax.StarlarkFile;
@@ -1420,24 +1418,12 @@ private void setPackageLocator(PathPackageLocator pkgLocator) {
       // We need to take additional steps to keep the corresponding data structures in sync.
       // (Some of the additional steps are carried out by ConfiguredTargetValueInvalidationListener,
       // and some by BuildView#buildHasIncompatiblePackageRoots and #updateSkyframe.)
-      artifactFactory.setSourceArtifactRoots(
-          createSourceArtifactRootMapOnNewPkgLocator(oldLocator, pkgLocator));
+      onPkgLocatorChange(oldLocator, pkgLocator);
     }
   }
 
-  protected ImmutableMap<Root, ArtifactRoot> createSourceArtifactRootMapOnNewPkgLocator(
-      PathPackageLocator oldLocator, PathPackageLocator pkgLocator) {
-    ImmutableMap.Builder<Root, ArtifactRoot> builder = ImmutableMap.builder();
-    Stream.concat(pkgLocator.getPathEntries().stream(), Stream.of(Root.absoluteRoot(fileSystem)))
-        .distinct()
-        .forEach(r -> builder.put(r, ArtifactRoot.asSourceRoot(r)));
-    Root externalRoot =
-        Root.fromPath(
-            directories.getOutputBase().getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION));
-    builder.put(externalRoot, ArtifactRoot.asExternalSourceRoot(externalRoot));
-
-    return builder.build();
-  }
+  protected abstract void onPkgLocatorChange(
+      PathPackageLocator oldLocator, PathPackageLocator pkgLocator);
 
   public SkyframeBuildView getSkyframeBuildView() {
     return skyframeBuildView;

src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java

@@ -99,15 +99,6 @@ private void setupRoots() {
     packageRootMap.put(barPackage, clientRoRoot);
     packageRootMap.put(alienPackage, alienRoot);
     artifactFactory.setPackageRoots(packageRootMap::get);
-    Root absoluteRoot = Root.absoluteRoot(clientRoot.asPath().getFileSystem());
-    artifactFactory.setSourceArtifactRoots(
-        ImmutableMap.of(
-            clientRoot,
-            ArtifactRoot.asSourceRoot(clientRoot),
-            clientRoRoot,
-            ArtifactRoot.asSourceRoot(clientRoRoot),
-            absoluteRoot,
-            ArtifactRoot.asSourceRoot(absoluteRoot)));
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java

@@ -65,7 +65,7 @@ public class ArtifactTest {
   @Before
   public final void setRootDir() throws Exception  {
     scratch = new Scratch();
-    execDir = scratch.dir("/exec");
+    execDir = scratch.dir("/base/exec");
     rootDir = ArtifactRoot.asDerivedRoot(execDir, "root");
   }
 
@@ -297,7 +297,6 @@ public void testCodecRecyclesSourceArtifactInstances() throws Exception {
     ArtifactRoot artifactRoot = ArtifactRoot.asSourceRoot(root);
     ArtifactFactory artifactFactory =
         new ArtifactFactory(execDir.getParentDirectory(), "blaze-out");
-    artifactFactory.setSourceArtifactRoots(ImmutableMap.of(root, artifactRoot));
     ArtifactResolverSupplier artifactResolverSupplierForTest =
         new ArtifactResolverSupplier() {
           @Override