Automated g4 rollback of commit 38b8350.

*** Reason for rollback *** Breaking Bazel build on linux, see http://ci.bazel.io/job/bazel-tests/733/ Repro: bazel build //src/test/java/com/google/devtools/build/lib:packages_test Found by bisecting. *** Original change description *** Only allocate some formerly frequently allocated PathFragment objects once. This reduces both gc churn and retained memory usage. RELNOTES: None PiperOrigin-RevId: 154821457
natansil · May 2, 2017 · 7beadb7 · 7beadb7
1 parent adb15a6
commit 7beadb7
Show file tree

Hide file tree

Showing 19 changed files with 44 additions and 104 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
@@ -543,7 +543,8 @@ public final PathFragment getRootRelativePath() {
    */
   public final PathFragment getRunfilesPath() {
     PathFragment relativePath = rootRelativePath;
-    if (relativePath.startsWith(Label.EXTERNAL_PATH_PREFIX)) {
+    if (relativePath.segmentCount() > 1
+        && relativePath.getSegment(0).equals(Label.EXTERNAL_PATH_PREFIX)) {
       // Turn external/repo/foo into ../repo/foo.
       relativePath = relativePath.relativeTo(Label.EXTERNAL_PATH_PREFIX);
       relativePath = PathFragment.create("..").getRelative(relativePath);

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -1260,18 +1260,7 @@ public Artifact getSingleSource() {
    * <p>For example "pkg/dir/name" -> "pkg/&lt;fragment>/rule/dir/name.
    */
   public final PathFragment getUniqueDirectory(String fragment) {
-    return getUniqueDirectory(PathFragment.create(fragment));
-  }
-
-  /**
-   * Returns a path fragment qualified by the rule name and unique fragment to
-   * disambiguate artifacts produced from the source file appearing in
-   * multiple rules.
-   *
-   * <p>For example "pkg/dir/name" -> "pkg/&lt;fragment>/rule/dir/name.
-   */
-  public final PathFragment getUniqueDirectory(PathFragment fragment) {
-    return AnalysisUtils.getUniqueDirectory(getLabel(), fragment);
+    return AnalysisUtils.getUniqueDirectory(getLabel(), PathFragment.create(fragment));
   }
 
   /**

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -1168,7 +1168,7 @@ private enum OutputDirectory {
     INCLUDE(BlazeDirectories.RELATIVE_INCLUDE_DIR),
     OUTPUT(false);
 
-    private final PathFragment nameFragment;
+    private final String name;
     private final boolean middleman;
 
     /**
@@ -1177,12 +1177,12 @@ private enum OutputDirectory {
      * @param isMiddleman whether the root should be a middleman root or a "normal" derived root.
      */
     OutputDirectory(boolean isMiddleman) {
-      this.nameFragment = PathFragment.EMPTY_FRAGMENT;
+      this.name = "";
       this.middleman = isMiddleman;
     }
 
     OutputDirectory(String name) {
-      this.nameFragment = PathFragment.create(name);
+      this.name = name;
       this.middleman = false;
     }
 
@@ -1198,28 +1198,15 @@ Root getRoot(
       }
       // e.g., [[execroot/repo1]/bazel-out/config/bin]
       return INTERNER.intern(
-          Root.asDerivedRoot(
-              execRoot,
-              outputDir.getRelative(nameFragment),
-              repositoryName.isMain()));
+          Root.asDerivedRoot(execRoot, outputDir.getRelative(name), repositoryName.isMain()));
     }
   }
 
-  private final BlazeDirectories directories;
-  private final String outputDirName;
-
-  // We intern the roots for non-main repositories, so we don't keep around thousands of copies of
-  // the same root.
+  // "Cache" of roots, so we don't keep around thousands of copies of the same root.
   private static Interner<Root> INTERNER = Interners.newWeakInterner();
 
-  // We precompute the roots for the main repository, since that's the common case.
-  private final Root outputDirectoryForMainRepository;
-  private final Root binDirectoryForMainRepository;
-  private final Root includeDirectoryForMainRepository;
-  private final Root genfilesDirectoryForMainRepository;
-  private final Root coverageDirectoryForMainRepository;
-  private final Root testlogsDirectoryForMainRepository;
-  private final Root middlemanDirectoryForMainRepository;
+  private final BlazeDirectories directories;
+  private final String outputDirName;
 
   /** If false, AnalysisEnviroment doesn't register any actions created by the ConfiguredTarget. */
   private final boolean actionsEnabled;
@@ -1457,22 +1444,6 @@ public BuildConfiguration(BlazeDirectories directories,
     this.mnemonic = buildMnemonic();
     this.outputDirName = (options.outputDirectoryName != null)
         ? options.outputDirectoryName : mnemonic;
-
-    this.outputDirectoryForMainRepository =
-        OutputDirectory.OUTPUT.getRoot(RepositoryName.MAIN, outputDirName, directories);
-    this.binDirectoryForMainRepository =
-        OutputDirectory.BIN.getRoot(RepositoryName.MAIN, outputDirName, directories);
-    this.includeDirectoryForMainRepository =
-        OutputDirectory.INCLUDE.getRoot(RepositoryName.MAIN, outputDirName, directories);
-    this.genfilesDirectoryForMainRepository =
-        OutputDirectory.GENFILES.getRoot(RepositoryName.MAIN, outputDirName, directories);
-    this.coverageDirectoryForMainRepository =
-        OutputDirectory.COVERAGE.getRoot(RepositoryName.MAIN, outputDirName, directories);
-    this.testlogsDirectoryForMainRepository =
-        OutputDirectory.TESTLOGS.getRoot(RepositoryName.MAIN, outputDirName, directories);
-    this.middlemanDirectoryForMainRepository =
-        OutputDirectory.MIDDLEMAN.getRoot(RepositoryName.MAIN, outputDirName, directories);
-
     this.platformName = buildPlatformName();
 
     this.shellExecutable = computeShellExecutable();
@@ -2097,9 +2068,7 @@ public String getPlatformName() {
    * Returns the output directory for this build configuration.
    */
   public Root getOutputDirectory(RepositoryName repositoryName) {
-    return repositoryName.equals(RepositoryName.MAIN)
-        ? outputDirectoryForMainRepository
-        : OutputDirectory.OUTPUT.getRoot(repositoryName, outputDirName, directories);
+    return OutputDirectory.OUTPUT.getRoot(repositoryName, outputDirName, directories);
   }
 
   /**
@@ -2118,9 +2087,7 @@ public Root getBinDirectory() {
    * repositories (external) but will need to be fixed.
    */
   public Root getBinDirectory(RepositoryName repositoryName) {
-    return repositoryName.equals(RepositoryName.MAIN)
-        ? binDirectoryForMainRepository
-        : OutputDirectory.BIN.getRoot(repositoryName, outputDirName, directories);
+    return OutputDirectory.BIN.getRoot(repositoryName, outputDirName, directories);
   }
 
   /**
@@ -2134,9 +2101,7 @@ public PathFragment getBinFragment() {
    * Returns the include directory for this build configuration.
    */
   public Root getIncludeDirectory(RepositoryName repositoryName) {
-    return repositoryName.equals(RepositoryName.MAIN)
-        ? includeDirectoryForMainRepository
-        : OutputDirectory.INCLUDE.getRoot(repositoryName, outputDirName, directories);
+    return OutputDirectory.INCLUDE.getRoot(repositoryName, outputDirName, directories);
   }
 
   /**
@@ -2149,9 +2114,7 @@ public Root getGenfilesDirectory() {
   }
 
   public Root getGenfilesDirectory(RepositoryName repositoryName) {
-    return repositoryName.equals(RepositoryName.MAIN)
-        ? genfilesDirectoryForMainRepository
-        : OutputDirectory.GENFILES.getRoot(repositoryName, outputDirName, directories);
+    return OutputDirectory.GENFILES.getRoot(repositoryName, outputDirName, directories);
   }
 
   /**
@@ -2160,18 +2123,14 @@ public Root getGenfilesDirectory(RepositoryName repositoryName) {
    * needed for Jacoco's coverage reporting tools.
    */
   public Root getCoverageMetadataDirectory(RepositoryName repositoryName) {
-    return repositoryName.equals(RepositoryName.MAIN)
-        ? coverageDirectoryForMainRepository
-        : OutputDirectory.COVERAGE.getRoot(repositoryName, outputDirName, directories);
+    return OutputDirectory.COVERAGE.getRoot(repositoryName, outputDirName, directories);
   }
 
   /**
    * Returns the testlogs directory for this build configuration.
    */
   public Root getTestLogsDirectory(RepositoryName repositoryName) {
-    return repositoryName.equals(RepositoryName.MAIN)
-        ? testlogsDirectoryForMainRepository
-        : OutputDirectory.TESTLOGS.getRoot(repositoryName, outputDirName, directories);
+    return OutputDirectory.TESTLOGS.getRoot(repositoryName, outputDirName, directories);
   }
 
   /**
@@ -2198,9 +2157,7 @@ public String getHostPathSeparator() {
    * Returns the internal directory (used for middlemen) for this build configuration.
    */
   public Root getMiddlemanDirectory(RepositoryName repositoryName) {
-    return repositoryName.equals(RepositoryName.MAIN)
-        ? middlemanDirectoryForMainRepository
-        : OutputDirectory.MIDDLEMAN.getRoot(repositoryName, outputDirName, directories);
+    return OutputDirectory.MIDDLEMAN.getRoot(repositoryName, outputDirName, directories);
   }
 
   public boolean getAllowRuntimeDepsOnNeverLink() {

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
@@ -207,7 +207,7 @@ public void postInitBinary(RuleContext ruleContext, RunfilesSupport runfilesSupp
   }
 
   private static boolean isUnderWorkspace(PathFragment path) {
-    return !path.startsWith(Label.EXTERNAL_PATH_PREFIX);
+    return !path.startsWith(PathFragment.create(Label.EXTERNAL_PATH_PREFIX));
   }
 
   private static String getZipRunfilesPath(PathFragment path, PathFragment workspaceName) {

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
@@ -72,7 +72,7 @@ public final class Label implements Comparable<Label>, Serializable, SkylarkPrin
   public static final PackageIdentifier EXTERNAL_PACKAGE_IDENTIFIER =
       PackageIdentifier.createInMainRepo(EXTERNAL_PACKAGE_NAME);
 
-  public static final PathFragment EXTERNAL_PATH_PREFIX = PathFragment.create("external");
+  public static final String EXTERNAL_PATH_PREFIX = "external";
 
   private static final Interner<Label> LABEL_INTERNER = BlazeInterners.newWeakInterner();
 

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
@@ -79,7 +79,7 @@ public static PackageIdentifier discoverFromExecPath(PathFragment execPath, bool
         ? Preconditions.checkNotNull(
             execPath.getParentDirectory(), "Must pass in files, not root directory")
         : execPath;
-    if (tofind.startsWith(Label.EXTERNAL_PATH_PREFIX)) {
+    if (tofind.startsWith(PathFragment.create(Label.EXTERNAL_PATH_PREFIX))) {
       // TODO(ulfjack): Remove this when kchodorow@'s exec root rearrangement has been rolled out.
       RepositoryName repository = RepositoryName.create("@" + tofind.getSegment(1));
       return PackageIdentifier.create(repository, tofind.subFragment(2, tofind.segmentCount()));

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
@@ -139,7 +139,7 @@ public static RepositoryName createFromValidStrippedName(String name) {
    * was invalid.
    */
   public static Pair<RepositoryName, PathFragment> fromPathFragment(PathFragment path) {
-    if (path.segmentCount() < 2 || !path.startsWith(Label.EXTERNAL_PATH_PREFIX)) {
+    if (path.segmentCount() < 2 || !path.getSegment(0).equals(Label.EXTERNAL_PATH_PREFIX)) {
       return null;
     }
     try {
@@ -202,7 +202,7 @@ public boolean isDefault() {
   }
 
   /**
-   * Returns if this is the main repository, that is, {@link #name} is "@".
+   * Returns if this is the default repository, that is, {@link #name} is "@".
    */
   public boolean isMain() {
     return name.equals("@");
@@ -232,7 +232,7 @@ public PathFragment getSourceRoot() {
   public PathFragment getPathUnderExecRoot() {
     return isDefault() || isMain()
         ? PathFragment.EMPTY_FRAGMENT
-        : Label.EXTERNAL_PATH_PREFIX.getRelative(strippedName());
+        : PathFragment.create(Label.EXTERNAL_PATH_PREFIX).getRelative(strippedName());
   }
 
   /**

diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java
@@ -45,7 +45,6 @@
  * filesystem) idempotent.
  */
 public class PathPackageLocator implements Serializable {
-  private static final PathFragment BUILD_PATH_FRAGMENT = PathFragment.create("BUILD");
 
   public static final ImmutableSet<String> DEFAULT_TOP_LEVEL_EXCLUDES =
       ImmutableSet.of("experimental");
@@ -102,8 +101,7 @@ public Path getPackageBuildFileNullable(PackageIdentifier packageIdentifier,
       AtomicReference<? extends UnixGlob.FilesystemCalls> cache)  {
     Preconditions.checkArgument(!packageIdentifier.getRepository().isDefault());
     if (packageIdentifier.getRepository().isMain()) {
-      return getFilePath(
-          packageIdentifier.getPackageFragment().getRelative(BUILD_PATH_FRAGMENT), cache);
+      return getFilePath(packageIdentifier.getPackageFragment().getRelative("BUILD"), cache);
     } else {
       Verify.verify(outputBase != null, String.format(
           "External package '%s' needs to be loaded but this PathPackageLocator instance does not "
@@ -113,7 +111,7 @@ public Path getPackageBuildFileNullable(PackageIdentifier packageIdentifier,
       // is true for the invocation in GlobCache, but not for the locator.getBuildFileForPackage()
       // invocation in Parser#include().
       Path buildFile = outputBase.getRelative(
-          packageIdentifier.getSourceRoot()).getRelative(BUILD_PATH_FRAGMENT);
+          packageIdentifier.getSourceRoot()).getRelative("BUILD");
       try {
         FileStatus stat = cache.get().statIfFound(buildFile, Symlinks.FOLLOW);
         if (stat != null && stat.isFile()) {

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -110,8 +110,6 @@ public Collection<Artifact> getInputsForIncludedFile(
         }
       };
 
-  private static final PathFragment BUILD_PATH_FRAGMENT = PathFragment.create("BUILD");
-
   private static final int VALIDATION_DEBUG = 0;  // 0==none, 1==warns/errors, 2==all
   private static final boolean VALIDATION_DEBUG_WARN = VALIDATION_DEBUG >= 1;
 
@@ -950,7 +948,7 @@ private static boolean isDeclaredIn(
     // Still not found: see if it is in a subdir of a declared package.
     Path root = input.getRoot().getPath();
     for (Path dir = input.getPath().getParentDirectory();;) {
-      if (dir.getRelative(BUILD_PATH_FRAGMENT).exists()) {
+      if (dir.getRelative("BUILD").exists()) {
         return false;  // Bad: this is a sub-package, not a subdir of a declared package.
       }
       dir = dir.getParentDirectory();

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -477,7 +477,7 @@ private static void verifyActionIncludePaths(CppCompileAction action, RuleContex
         continue;
       }
       // One starting ../ is okay for getting to a sibling repository.
-      if (include.startsWith(Label.EXTERNAL_PATH_PREFIX)) {
+      if (include.startsWith(PathFragment.create(Label.EXTERNAL_PATH_PREFIX))) {
         include = include.relativeTo(Label.EXTERNAL_PATH_PREFIX);
       }
       if (include.isAbsolute()

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
@@ -60,8 +60,6 @@
  * <p>This class can be used only after the loading phase.
  */
 public class CppHelper {
-  static final PathFragment OBJS = PathFragment.create("_objs");
-
   private static final String GREPPED_INCLUDES_SUFFIX = ".includes";
 
   // TODO(bazel-team): should this use Link.SHARED_LIBRARY_FILETYPES?
@@ -285,7 +283,7 @@ public static CcToolchainProvider getToolchain(RuleContext ruleContext,
    * Returns the directory where object files are created.
    */
   public static PathFragment getObjDirectory(Label ruleLabel) {
-    return AnalysisUtils.getUniqueDirectory(ruleLabel, OBJS);
+    return AnalysisUtils.getUniqueDirectory(ruleLabel, PathFragment.create("_objs"));
   }
 
   /**

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -952,7 +952,7 @@ public static ImmutableMap<Artifact, Artifact> mapLinkstampsToOutputs(
     PathFragment stampOutputDirectory =
         outputBinaryPath
             .getParentDirectory()
-            .getRelative(CppHelper.OBJS)
+            .getRelative("_objs")
             .getRelative(outputBinaryPath.getBaseName());
 
     for (Artifact linkstamp : linkstamps) {

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java
@@ -71,7 +71,6 @@ public final class JavaCompilationHelper {
   private final ImmutableList<Artifact> additionalJavaBaseInputs;
 
   private static final String DEFAULT_ATTRIBUTES_SUFFIX = "";
-  private static final PathFragment JAVAC = PathFragment.create("_javac");
 
   public JavaCompilationHelper(RuleContext ruleContext, JavaSemantics semantics,
       ImmutableList<String> javacOpts, JavaTargetAttributes.Builder attributes,
@@ -565,7 +564,7 @@ private PathFragment classDir(Artifact outputJar) {
   private PathFragment workDir(Artifact outputJar, String suffix) {
     String basename = FileSystemUtils.removeExtension(outputJar.getExecPath().getBaseName());
     return getConfiguration().getBinDirectory(ruleContext.getRule().getRepository()).getExecPath()
-        .getRelative(ruleContext.getUniqueDirectory(JAVAC))
+        .getRelative(ruleContext.getUniqueDirectory("_javac"))
         .getRelative(basename + suffix);
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/IntermediateArtifacts.java b/src/main/java/com/google/devtools/build/lib/rules/objc/IntermediateArtifacts.java
@@ -34,8 +34,6 @@
 // TODO(bazel-team): This should really be named DerivedArtifacts as it contains methods for
 // final as well as intermediate artifacts.
 public final class IntermediateArtifacts {
-  private static final PathFragment OBJS = PathFragment.create("_objs");
-
   static final String LINKMAP_SUFFIX = ".linkmap";
 
   /**
@@ -277,7 +275,8 @@ public Artifact archive() {
   }
 
   private Artifact inUniqueObjsDir(Artifact source, String extension) {
-    PathFragment uniqueDir = OBJS.getRelative(ruleContext.getLabel().getName());
+    PathFragment uniqueDir =
+        PathFragment.create("_objs").getRelative(ruleContext.getLabel().getName());
     PathFragment sourceFile = uniqueDir.getRelative(source.getRootRelativePath());
     PathFragment scopeRelativePath = FileSystemUtils.replaceExtension(sourceFile, extension);
     return scopedArtifact(scopeRelativePath);

diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java
@@ -100,7 +100,7 @@ protected static FileValue getWorkspaceFile(RootedPath directory, Environment en
             directory.getRoot(),
             directory
                 .getRelativePath()
-                .getRelative(PackageLookupValue.BuildFileName.WORKSPACE.getFilenameFragment()));
+                .getChild(PackageLookupValue.BuildFileName.WORKSPACE.getFilename()));
 
     SkyKey workspaceFileKey = FileValue.key(workspaceRootedFile);
     FileValue value;

diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
@@ -473,7 +473,8 @@ public static void addExternalFilesDependencies(
 
       if (repositoryPath.segmentCount() > 1) {
         if (rule.getRuleClass().equals(LocalRepositoryRule.NAME)
-            && repositoryPath.endsWith(BuildFileName.WORKSPACE.getFilenameFragment())) {
+            && repositoryPath.endsWith(
+                PathFragment.create(BuildFileName.WORKSPACE.getFilename()))) {
           // Ignore this, there is a dependency from LocalRepositoryFunction->WORKSPACE file already
           return;
         }

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunction.java
@@ -94,7 +94,7 @@ private Optional<Boolean> maybeGetWorkspaceFileExistence(Environment env, Rooted
               directory.getRoot(),
               directory
                   .getRelativePath()
-                  .getRelative(PackageLookupValue.BuildFileName.WORKSPACE.getFilenameFragment()));
+                  .getChild(PackageLookupValue.BuildFileName.WORKSPACE.getFilename()));
       FileValue workspaceFileValue =
           (FileValue)
               env.getValueOrThrow(

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
@@ -315,7 +315,7 @@ private PackageLookupValue computeExternalPackageLookupValue(
     // This checks for the build file names in the correct precedence order.
     for (BuildFileName buildFileName : buildFilesByPriority) {
       PathFragment buildFileFragment =
-          id.getPackageFragment().getRelative(buildFileName.getFilenameFragment());
+          id.getPackageFragment().getChild(buildFileName.getFilename());
       RootedPath buildFileRootedPath =
           RootedPath.toRootedPath(repositoryValue.getPath(), buildFileFragment);
       FileValue fileValue = getFileValue(buildFileRootedPath, env, packageIdentifier);