Remove all callers of ArtifactRoot.asSourcePath from production code besides the ones in SkyframeExecutor, called once for each Root in the package path list. This ensures there is a single canonical ArtifactRoot for each source root.

It is still the case that every Package loaded has its own Root (https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/packages/Package.java;l=287?q=Package.java), which is a waste of memory, but because of the map inside ArtifactFactory introduced in this change, all Artifacts with the same logical source root share a single ArtifactRoot instance.

PiperOrigin-RevId: 192513819

4 files changed, 31 insertions, 33 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
index 247518570a..331401fb3e 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
@@ -34,7 +34,6 @@ import com.google.devtools.build.lib.vfs.Root;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Map.Entry;
-import javax.annotation.Nullable;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -52,9 +51,9 @@ public class ArtifactFactoryTest {
   private Scratch scratch = new Scratch();
 
   private Path execRoot;
-  private ArtifactRoot clientRoot;
-  private ArtifactRoot clientRoRoot;
-  private ArtifactRoot alienRoot;
+  private Root clientRoot;
+  private Root clientRoRoot;
+  private Root alienRoot;
   private ArtifactRoot outRoot;
 
   private PathFragment fooPath;
@@ -75,9 +74,9 @@ public class ArtifactFactoryTest {
   @Before
   public final void createFiles() throws Exception  {
     execRoot = scratch.dir("/output/workspace");
-    clientRoot = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.dir("/client/workspace")));
-    clientRoRoot = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.dir("/client/RO/workspace")));
-    alienRoot = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.dir("/client/workspace")));
+    clientRoot = Root.fromPath(scratch.dir("/client/workspace"));
+    clientRoRoot = Root.fromPath(scratch.dir("/client/RO/workspace"));
+    alienRoot = Root.fromPath(scratch.dir("/client/workspace"));
     outRoot = ArtifactRoot.asDerivedRoot(execRoot, execRoot.getRelative("out-root/x/bin"));
 
     fooPath = PathFragment.create("foo");
@@ -97,11 +96,20 @@ public class ArtifactFactoryTest {
   }
 
   private void setupRoots() {
-    Map<PackageIdentifier, ArtifactRoot> packageRootMap = new HashMap<>();
+    Map<PackageIdentifier, Root> packageRootMap = new HashMap<>();
     packageRootMap.put(fooPackage, clientRoot);
     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
@@ -157,7 +165,7 @@ public class ArtifactFactoryTest {
   public void testResolveArtifactWithUpLevelFailsCleanly() throws Exception {
     // We need a package in the root directory to make every exec path (even one with up-level
     // references) be in a package.
-    Map<PackageIdentifier, ArtifactRoot> packageRoots =
+    Map<PackageIdentifier, Root> packageRoots =
         ImmutableMap.of(PackageIdentifier.createInMainRepo(PathFragment.create("")), clientRoot);
     artifactFactory.setPackageRoots(packageRoots::get);
     PathFragment outsideWorkspace = PathFragment.create("../foo");
@@ -194,11 +202,10 @@ public class ArtifactFactoryTest {
 
   @Test
   public void testAbsoluteArtifact() throws Exception {
-    ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(execRoot));
-    ArtifactRoot absoluteRoot =
-        ArtifactRoot.asSourceRoot(Root.absoluteRoot(scratch.getFileSystem()));
+    Root absoluteRoot = Root.absoluteRoot(scratch.getFileSystem());
 
-    assertThat(artifactFactory.getSourceArtifact(PathFragment.create("foo"), root).getExecPath())
+    assertThat(
+            artifactFactory.getSourceArtifact(PathFragment.create("foo"), clientRoot).getExecPath())
         .isEqualTo(PathFragment.create("foo"));
     assertThat(
             artifactFactory
@@ -207,7 +214,7 @@ public class ArtifactFactoryTest {
         .isEqualTo(PathFragment.create("/foo"));
     MoreAsserts.assertThrows(
         IllegalArgumentException.class,
-        () -> artifactFactory.getSourceArtifact(PathFragment.create("/foo"), root));
+        () -> artifactFactory.getSourceArtifact(PathFragment.create("/foo"), clientRoot));
     MoreAsserts.assertThrows(
         IllegalArgumentException.class,
         () -> artifactFactory.getSourceArtifact(PathFragment.create("foo"), absoluteRoot));
@@ -233,18 +240,17 @@ public class ArtifactFactoryTest {
   }
 
   private static class MockPackageRootResolver implements PackageRootResolver {
-    private Map<PathFragment, ArtifactRoot> packageRoots = Maps.newHashMap();
+    private final Map<PathFragment, Root> packageRoots = Maps.newHashMap();
 
-    public void setPackageRoots(Map<PackageIdentifier, ArtifactRoot> packageRoots) {
-      for (Entry<PackageIdentifier, ArtifactRoot> packageRoot : packageRoots.entrySet()) {
+    public void setPackageRoots(Map<PackageIdentifier, Root> packageRoots) {
+      for (Entry<PackageIdentifier, Root> packageRoot : packageRoots.entrySet()) {
         this.packageRoots.put(packageRoot.getKey().getPackageFragment(), packageRoot.getValue());
       }
     }
 
     @Override
-    public Map<PathFragment, ArtifactRoot> findPackageRootsForFiles(
-        Iterable<PathFragment> execPaths) {
-      Map<PathFragment, ArtifactRoot> result = new HashMap<>();
+    public Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> execPaths) {
+      Map<PathFragment, Root> result = new HashMap<>();
       for (PathFragment execPath : execPaths) {
         for (PathFragment dir = execPath.getParentDirectory(); dir != null;
             dir = dir.getParentDirectory()) {
@@ -258,11 +264,5 @@ public class ArtifactFactoryTest {
       }
       return result;
     }
-
-    @Override
-    @Nullable
-    public Map<PathFragment, ArtifactRoot> findPackageRoots(Iterable<PathFragment> execPaths) {
-      return null; // unused
-    }
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
index b9330d4434..49a6aa0607 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
@@ -669,13 +669,12 @@ public final class ActionsTestUtil {
    */
   public static class FakeArtifactResolverBase implements ArtifactResolver {
     @Override
-    public Artifact getSourceArtifact(
-        PathFragment execPath, ArtifactRoot root, ArtifactOwner owner) {
+    public Artifact getSourceArtifact(PathFragment execPath, Root root, ArtifactOwner owner) {
       throw new UnsupportedOperationException();
     }
 
     @Override
-    public Artifact getSourceArtifact(PathFragment execPath, ArtifactRoot root) {
+    public Artifact getSourceArtifact(PathFragment execPath, Root root) {
       throw new UnsupportedOperationException();
     }
 
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 7129779d58..5ade0d4d3e 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -1056,13 +1056,12 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
         .isSameAs(getGeneratingActionForLabel(labelA));
   }
 
-  protected Artifact getSourceArtifact(PathFragment rootRelativePath, ArtifactRoot root) {
+  protected Artifact getSourceArtifact(PathFragment rootRelativePath, Root root) {
     return view.getArtifactFactory().getSourceArtifact(rootRelativePath, root);
   }
 
   protected Artifact getSourceArtifact(String name) {
-    return getSourceArtifact(
-        PathFragment.create(name), ArtifactRoot.asSourceRoot(Root.fromPath(rootDirectory)));
+    return getSourceArtifact(PathFragment.create(name), Root.fromPath(rootDirectory));
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/HeaderThinningTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/HeaderThinningTest.java
index ee9b9fb6d8..e7bfb84653 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/HeaderThinningTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/HeaderThinningTest.java
@@ -175,7 +175,7 @@ public class HeaderThinningTest extends ObjcRuleTestCase {
   private Artifact getHeadersListArtifact(Artifact sourceFile) {
     return getSourceArtifact(
         FileSystemUtils.replaceExtension(sourceFile.getExecPath(), ".headers_list"),
-        sourceFile.getRoot());
+        sourceFile.getRoot().getRoot());
   }
 
   private static Map<PathFragment, Artifact> createHeaderFilesMap(Iterable<Artifact> artifacts) {