Don't create-and-retain a PathFragment instance for suffix for each Runfiles instance.

In practice, `suffix` is always the workspace name string, so all these `PathFragment`s are wasteful dupes. Instead, we create the `PathFragment` on the fly (and then throw it away) when needed. Thus this is essentially a logical rollback of unknown commit (in other words, that CL was actually a memory regression).

As an alternative, I considered threading through a logical singleton `PathFragment` to all `Runfiles` instances (e.g. from `RuleClassProvider`) but that had a much larger blast radius.

While I'm here, rename `suffix` to `prefix`. Afaict, the code actually means to say "prefix" and "suffix" was a mistake in unknown commit.

PiperOrigin-RevId: 671429745
Change-Id: I1e72588859ceab39bcd3f05fa8760aa0d7096b79

Author: haxorz

src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java

@@ -118,9 +118,9 @@ public void fingerprint(Fingerprint fp) {
    *
    * <p>Using "foo" will put runfiles under &lt;target&gt;.runfiles/foo.
    *
-   * <p>This is either set to the workspace name, or is empty.
+   * <p>This is either set to the workspace name, or the empty string.
    */
-  private final PathFragment suffix;
+  private final String prefix;
 
   /**
    * The artifacts that should be present in the runfiles directory.
@@ -194,14 +194,14 @@ public enum ConflictPolicy {
   private final boolean legacyExternalRunfiles;
 
   private Runfiles(
-      PathFragment suffix,
+      String prefix,
       NestedSet<Artifact> artifacts,
       NestedSet<SymlinkEntry> symlinks,
       NestedSet<SymlinkEntry> rootSymlinks,
       EmptyFilesSupplier emptyFilesSupplier,
       ConflictPolicy conflictPolicy,
       boolean legacyExternalRunfiles) {
-    this.suffix = suffix;
+    this.prefix = prefix;
     this.artifacts = Preconditions.checkNotNull(artifacts);
     this.symlinks = Preconditions.checkNotNull(symlinks);
     this.rootSymlinks = Preconditions.checkNotNull(rootSymlinks);
@@ -215,11 +215,9 @@ public boolean isImmutable() {
     return true; // immutable and Starlark-hashable
   }
 
-  /**
-   * Returns the runfiles' suffix.
-   */
-  public PathFragment getSuffix() {
-    return suffix;
+  /** Returns the runfiles' prefix. This is the same as the workspace name. */
+  public String getPrefix() {
+    return prefix;
   }
 
   /** Returns the collection of runfiles as artifacts. */
@@ -366,7 +364,8 @@ public Map<PathFragment, Artifact> getRunfilesInputs(
     // Copy manifest map to another manifest map, prepending the workspace name to every path.
     // E.g. for workspace "myworkspace", the runfile entry "mylib.so"->"/path/to/mylib.so" becomes
     // "myworkspace/mylib.so"->"/path/to/mylib.so".
-    ManifestBuilder builder = new ManifestBuilder(suffix, legacyExternalRunfiles);
+    ManifestBuilder builder =
+        new ManifestBuilder(PathFragment.create(prefix), legacyExternalRunfiles);
     builder.addUnderWorkspace(manifest, checker);
 
     // Finally add symlinks relative to the root of the runfiles tree, on top of everything else.
@@ -640,7 +639,7 @@ public void put(Map<PathFragment, Artifact> map, PathFragment path, Artifact art
   public static final class Builder {
 
     /** This is set to the workspace name */
-    private final PathFragment suffix;
+    private final String prefix;
 
     /**
      * This must be COMPILE_ORDER because {@link #asMapWithoutRootSymlinks} overwrites earlier
@@ -662,7 +661,7 @@ public static final class Builder {
      * Only used for Runfiles.EMPTY.
      */
     private Builder() {
-      this.suffix = PathFragment.EMPTY_FRAGMENT;
+      this.prefix = "";
       this.legacyExternalRunfiles = false;
     }
 
@@ -678,23 +677,12 @@ public Builder(String workspace) {
 
     /**
      * Creates a builder with the given suffix.
-     * @param workspace is the string specified in workspace() in the WORKSPACE file.
-     * @param legacyExternalRunfiles if the wsname/external/repo symlinks should also be
-     *     created.
-     */
-    public Builder(String workspace, boolean legacyExternalRunfiles) {
-      this(PathFragment.create(workspace), legacyExternalRunfiles);
-    }
-
-    /**
-     * Creates a builder with the given suffix.
-     * @param suffix is the PathFragment wrapping the string specified in workspace() in the
-     *     WORKSPACE file.
-     * @param legacyExternalRunfiles if the wsname/external/repo symlinks should also be
-     *     created.
+     *
+     * @param prefix is the string specified in workspace() in the WORKSPACE file.
+     * @param legacyExternalRunfiles if the wsname/external/repo symlinks should also be created.
      */
-    private Builder(PathFragment suffix, boolean legacyExternalRunfiles) {
-      this.suffix = suffix;
+    public Builder(String prefix, boolean legacyExternalRunfiles) {
+      this.prefix = prefix;
       this.legacyExternalRunfiles = legacyExternalRunfiles;
     }
 
@@ -703,7 +691,7 @@ private Builder(PathFragment suffix, boolean legacyExternalRunfiles) {
      */
     public Runfiles build() {
       return new Runfiles(
-          suffix,
+          prefix,
           artifactsBuilder.build(),
           symlinksBuilder.build(),
           rootSymlinksBuilder.build(),
@@ -832,10 +820,10 @@ public Builder merge(Runfiles runfiles) {
       if (runfiles.isEmpty()) {
         return this;
       }
-      // The suffix should be the same within any blaze build, except for the EMPTY runfiles, which
-      // may have an empty suffix, but that is covered above.
+      // The prefix should be the same within any blaze build, except for the EMPTY runfiles, which
+      // may have an empty prefix, but that is covered above.
       Preconditions.checkArgument(
-          suffix.equals(runfiles.suffix), "%s != %s", suffix, runfiles.suffix);
+          prefix.equals(runfiles.prefix), "%s != %s", prefix, runfiles.prefix);
       artifactsBuilder.addTransitive(runfiles.getArtifacts());
       symlinksBuilder.addTransitive(runfiles.getSymlinks());
       rootSymlinksBuilder.addTransitive(runfiles.getRootSymlinks());
@@ -1026,7 +1014,7 @@ public Runfiles merge(RunfilesApi other, StarlarkThread thread) throws EvalExcep
     } else if (o.isEmpty()) {
       return this;
     }
-    return new Runfiles.Builder(suffix, false)
+    return new Runfiles.Builder(prefix, false)
         .merge(this)
         .merge(o)
         .build()
@@ -1044,15 +1032,15 @@ public Runfiles mergeAll(Sequence<?> sequence, StarlarkThread thread) throws Eva
     // and `x.merge(y)` in Starlark.
     Runfiles uniqueNonEmptyMergee = null;
     if (!this.isEmpty()) {
-      builder = new Builder(suffix, false).merge(this);
+      builder = new Builder(prefix, false).merge(this);
       uniqueNonEmptyMergee = this;
     }
 
     Sequence<Runfiles> runfilesSequence = Sequence.cast(sequence, Runfiles.class, "param");
     for (Runfiles runfiles : runfilesSequence) {
       if (!runfiles.isEmpty()) {
         if (builder == null) {
-          builder = new Builder(runfiles.suffix, /* legacyExternalRunfiles = */ false);
+          builder = new Builder(runfiles.prefix, /* legacyExternalRunfiles= */ false);
           uniqueNonEmptyMergee = runfiles;
         } else {
           uniqueNonEmptyMergee = null;
@@ -1075,7 +1063,7 @@ public void fingerprint(
       ActionKeyContext actionKeyContext, Fingerprint fp, boolean digestAbsolutePaths) {
     fp.addInt(conflictPolicy.ordinal());
     fp.addBoolean(legacyExternalRunfiles);
-    fp.addPath(suffix);
+    fp.addString(prefix);
 
     actionKeyContext.addNestedSetToFingerprint(SYMLINK_ENTRY_MAP_FN, fp, symlinks);
     actionKeyContext.addNestedSetToFingerprint(SYMLINK_ENTRY_MAP_FN, fp, rootSymlinks);
@@ -1094,7 +1082,7 @@ public void fingerprint(
   String describeFingerprint(boolean digestAbsolutePaths) {
     return String.format("conflictPolicy: %s\n", conflictPolicy)
         + String.format("legacyExternalRunfiles: %s\n", legacyExternalRunfiles)
-        + String.format("suffix: %s\n", suffix)
+        + String.format("prefix: %s\n", prefix)
         + String.format(
             "symlinks: %s\n", describeNestedSetFingerprint(SYMLINK_ENTRY_MAP_FN, symlinks))
         + String.format(

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

@@ -175,7 +175,7 @@ public boolean isBuildRunfileLinks() {
 
     @Override
     public String getWorkspaceName() {
-      return runfiles.getSuffix().getPathString();
+      return runfiles.getPrefix();
     }
   }
 

src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java

@@ -926,7 +926,7 @@ private static Path ensureRunfilesBuilt(
     // On Windows, runfiles tree is disabled.
     // Workspace name directory doesn't exist, so don't add it.
     if (configuration.runfilesEnabled()) {
-      workingDir = workingDir.getRelative(runfilesSupport.getRunfiles().getSuffix());
+      workingDir = workingDir.getRelative(runfilesSupport.getRunfiles().getPrefix());
     }
 
     // Always create runfiles directory and the workspace-named directory underneath, even if we

src/test/java/com/google/devtools/build/lib/analysis/util/FakeRunfilesTree.java

@@ -88,6 +88,6 @@ public boolean isBuildRunfileLinks() {
 
   @Override
   public String getWorkspaceName() {
-    return runfiles.getSuffix().getPathString();
+    return runfiles.getPrefix();
   }
 }