(IjProject) Do not hash folders

Summary:
Enabling aggregation of modules with Android resources uncovered poor performance of `buck project` while storing folders in a set.

To solve this performance problem sets are replaced with lists. This does not affect the correctness of the algorithm because of the way these folders are constructed. Also, I added a check to make sure the same folders aren't created.

Test Plan: Unit tests and local testing. Make sure enabling aggregation resource modules doesn't affect run time.

Reviewed By: JonShemitz

fbshipit-source-id: 55e0f0c

src/com/facebook/buck/ide/intellij/IjProjectTemplateDataPreparer.java

@@ -38,6 +38,8 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableCollection;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
@@ -181,23 +183,24 @@ public ImmutableSet<IjLibrary> getLibrariesToBeWritten() {
   private ContentRoot createContentRoot(
       final IjModule module,
       Path contentRootPath,
-      ImmutableSet<IjFolder> folders,
+      ImmutableCollection<IjFolder> folders,
       final Path moduleLocationBasePath) {
     String url = IjProjectPaths.toModuleDirRelativeString(contentRootPath, moduleLocationBasePath);
-    ImmutableSet<IjFolder> simplifiedFolders =
+    ImmutableCollection<IjFolder> simplifiedFolders =
         sourceRootSimplifier.simplify(contentRootPath.getNameCount(), folders);
     IjFolderToIjSourceFolderTransform transformToFolder =
         new IjFolderToIjSourceFolderTransform(module);
-    ImmutableSortedSet<IjSourceFolder> sourceFolders =
+    ImmutableList<IjSourceFolder> sourceFolders =
         simplifiedFolders
             .stream()
             .map(transformToFolder::apply)
-            .collect(MoreCollectors.toImmutableSortedSet(Ordering.natural()));
+            .sorted()
+            .collect(MoreCollectors.toImmutableList());
     return ContentRoot.builder().setUrl(url).setFolders(sourceFolders).build();
   }
 
-  public ImmutableSet<IjFolder> createExcludes(final IjModule module) throws IOException {
-    final ImmutableSet.Builder<IjFolder> excludesBuilder = ImmutableSet.builder();
+  public ImmutableCollection<IjFolder> createExcludes(final IjModule module) throws IOException {
+    final ImmutableList.Builder<IjFolder> excludesBuilder = ImmutableList.builder();
     final Path moduleBasePath = module.getModuleBasePath();
     projectFilesystem.walkRelativeFileTree(
         moduleBasePath,
@@ -261,9 +264,10 @@ public ContentRoot getContentRoot(IjModule module) throws IOException {
     Path moduleLocation = module.getModuleImlFilePath();
     final Path moduleLocationBasePath =
         (moduleLocation.getParent() == null) ? Paths.get("") : moduleLocation.getParent();
-    ImmutableSet<IjFolder> sourcesAndExcludes =
+    ImmutableList<IjFolder> sourcesAndExcludes =
         Stream.concat(module.getFolders().stream(), createExcludes(module).stream())
-            .collect(MoreCollectors.toImmutableSortedSet());
+            .sorted()
+            .collect(MoreCollectors.toImmutableList());
     return createContentRoot(module, moduleBasePath, sourcesAndExcludes, moduleLocationBasePath);
   }
 

src/com/facebook/buck/ide/intellij/IjSourceRootSimplifier.java

@@ -27,8 +27,8 @@
 import com.facebook.buck.jvm.core.JavaPackageFinder;
 import com.facebook.buck.util.MoreCollectors;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
 import java.nio.file.Path;
 import java.util.Collection;
 import java.util.HashMap;
@@ -57,7 +57,8 @@ public IjSourceRootSimplifier(JavaPackageFinder javaPackageFinder) {
    * @param folders set of {@link IjFolder}s to simplify.
    * @return simplified set of {@link IjFolder}s.
    */
-  public ImmutableSet<IjFolder> simplify(int simplificationLimit, ImmutableSet<IjFolder> folders) {
+  public ImmutableCollection<IjFolder> simplify(
+      int simplificationLimit, Iterable<IjFolder> folders) {
     PackagePathCache packagePathCache = new PackagePathCache(folders, javaPackageFinder);
     BottomUpPathMerger walker =
         new BottomUpPathMerger(folders, simplificationLimit, packagePathCache);
@@ -102,11 +103,11 @@ public BottomUpPathMerger(
       }
     }
 
-    public ImmutableSet<IjFolder> getMergedFolders() {
+    private ImmutableCollection<IjFolder> getMergedFolders() {
       for (Path topLevel : tree.getNodesWithNoIncomingEdges()) {
         walk(topLevel);
       }
-      return ImmutableSet.copyOf(mergePathsMap.values());
+      return ImmutableList.copyOf(mergePathsMap.values());
     }
 
     /**
@@ -126,12 +127,12 @@ private Optional<IjFolder> walk(Path currentPath) {
               .map(this::walk)
               .collect(MoreCollectors.toImmutableList());
 
-      ImmutableSet<IjFolder> presentChildren =
+      ImmutableList<IjFolder> presentChildren =
           children
               .stream()
               .filter(Optional::isPresent)
               .map(Optional::get)
-              .collect(MoreCollectors.toImmutableSet());
+              .collect(MoreCollectors.toImmutableList());
 
       IjFolder currentFolder = mergePathsMap.get(currentPath);
       if (presentChildren.isEmpty()) {
@@ -148,7 +149,7 @@ private Optional<IjFolder> walk(Path currentPath) {
     private Optional<IjFolder> tryMergingParentAndChildren(
         Path currentPath,
         @Nullable IjFolder parentFolder,
-        ImmutableSet<IjFolder> children,
+        ImmutableCollection<IjFolder> children,
         boolean hasNonPresentChildren) {
       if (parentFolder == null) {
         return mergeChildrenIntoNewParentFolder(currentPath, children);
@@ -179,7 +180,7 @@ private Optional<IjFolder> tryMergingParentAndChildren(
      *
      * <p>The best type in this algorithm is the type with the maximum number of children.
      */
-    private FolderTypeWithPackageInfo findBestFolderType(ImmutableSet<IjFolder> children) {
+    private FolderTypeWithPackageInfo findBestFolderType(ImmutableCollection<IjFolder> children) {
       if (children.size() == 1) {
         return FolderTypeWithPackageInfo.fromFolder(children.iterator().next());
       }
@@ -210,14 +211,14 @@ private FolderTypeWithPackageInfo findBestFolderType(ImmutableSet<IjFolder> chil
      * <p>The type of the result folder depends on the children.
      */
     private Optional<IjFolder> mergeChildrenIntoNewParentFolder(
-        Path currentPath, ImmutableSet<IjFolder> children) {
-      ImmutableSet<IjFolder> childrenToMerge =
+        Path currentPath, ImmutableCollection<IjFolder> children) {
+      ImmutableList<IjFolder> childrenToMerge =
           children
               .stream()
               .filter(
                   child ->
                       SourceFolder.class.isInstance(child) || TestFolder.class.isInstance(child))
-              .collect(MoreCollectors.toImmutableSet());
+              .collect(MoreCollectors.toImmutableList());
 
       if (childrenToMerge.isEmpty()) {
         return Optional.empty();
@@ -238,14 +239,14 @@ private Optional<IjFolder> mergeChildrenIntoNewParentFolder(
     private Optional<IjFolder> tryCreateNewParentFolderFromChildrenWithoutPackages(
         FolderTypeWithPackageInfo typeForMerging,
         Path currentPath,
-        ImmutableSet<IjFolder> children) {
+        ImmutableCollection<IjFolder> children) {
       Class<? extends IjFolder> folderClass = typeForMerging.getFolderTypeClass();
-      ImmutableSet<IjFolder> childrenToMerge =
+      ImmutableList<IjFolder> childrenToMerge =
           children
               .stream()
               .filter(folderClass::isInstance)
               .filter(folder -> !folder.getWantsPackagePrefix())
-              .collect(MoreCollectors.toImmutableSet());
+              .collect(MoreCollectors.toImmutableList());
 
       if (childrenToMerge.isEmpty()) {
         return Optional.empty();
@@ -272,14 +273,14 @@ private Optional<IjFolder> tryCreateNewParentFolderFromChildrenWithoutPackages(
     private Optional<IjFolder> tryCreateNewParentFolderFromChildrenWithPackage(
         FolderTypeWithPackageInfo typeForMerging,
         Path currentPath,
-        ImmutableSet<IjFolder> children) {
+        ImmutableCollection<IjFolder> children) {
       Optional<Path> currentPackage = packagePathCache.lookup(currentPath);
       if (!currentPackage.isPresent()) {
         return Optional.empty();
       }
 
       Class<? extends IjFolder> folderClass = typeForMerging.getFolderTypeClass();
-      ImmutableSet<IjFolder> childrenToMerge =
+      ImmutableList<IjFolder> childrenToMerge =
           children
               .stream()
               .filter(folderClass::isInstance)
@@ -288,7 +289,7 @@ private Optional<IjFolder> tryCreateNewParentFolderFromChildrenWithPackage(
                   child ->
                       canMergeWithKeepingPackage(
                           currentPath, currentPackage.get(), child, packagePathCache))
-              .collect(MoreCollectors.toImmutableSet());
+              .collect(MoreCollectors.toImmutableList());
 
       if (childrenToMerge.isEmpty()) {
         return Optional.empty();
@@ -330,13 +331,13 @@ private Optional<IjFolder> tryCreateNewParentFolderFromChildrenWithPackage(
      * </pre>
      */
     private Optional<IjFolder> mergeFoldersWithMatchingPackageIntoParent(
-        IjFolder parentFolder, ImmutableSet<IjFolder> children) {
+        IjFolder parentFolder, ImmutableCollection<IjFolder> children) {
 
-      ImmutableSet<IjFolder> childrenToMerge =
+      ImmutableList<IjFolder> childrenToMerge =
           children
               .stream()
               .filter(child -> canMergeWithKeepingPackage(parentFolder, child, packagePathCache))
-              .collect(MoreCollectors.toImmutableSet());
+              .collect(MoreCollectors.toImmutableList());
 
       IjFolder result = mergeFolders(parentFolder, childrenToMerge);
 
@@ -348,12 +349,12 @@ private Optional<IjFolder> mergeFoldersWithMatchingPackageIntoParent(
 
     /** Merges children that can be merged into a parent. */
     private Optional<IjFolder> mergeAndRemoveSimilarChildren(
-        IjFolder parentFolder, ImmutableSet<IjFolder> children) {
-      ImmutableSet<IjFolder> childrenToMerge =
+        IjFolder parentFolder, ImmutableCollection<IjFolder> children) {
+      ImmutableList<IjFolder> childrenToMerge =
           children
               .stream()
               .filter(folder -> folder.canMergeWith(parentFolder))
-              .collect(MoreCollectors.toImmutableSet());
+              .collect(MoreCollectors.toImmutableList());
 
       IjFolder result = mergeFolders(parentFolder, childrenToMerge);
 
@@ -434,7 +435,7 @@ private static class PackagePathCache {
     JavaPackagePathCache delegate;
 
     public PackagePathCache(
-        ImmutableSet<IjFolder> startingFolders, JavaPackageFinder javaPackageFinder) {
+        Iterable<IjFolder> startingFolders, JavaPackageFinder javaPackageFinder) {
       delegate = new JavaPackagePathCache();
       for (IjFolder startingFolder : startingFolders) {
         if (!startingFolder.getWantsPackagePrefix()) {

src/com/facebook/buck/ide/intellij/ModuleBuildContext.java

@@ -27,6 +27,8 @@
 import com.facebook.buck.rules.TargetNode;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.HashMultimap;
+import com.google.common.collect.ImmutableCollection;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Multimap;
@@ -42,7 +44,7 @@ public class ModuleBuildContext {
 
   private Optional<IjModuleAndroidFacet.Builder> androidFacetBuilder;
   private ImmutableSet.Builder<Path> extraClassPathDependenciesBuilder;
-  private ImmutableSet.Builder<IjFolder> generatedSourceCodeFoldersBuilder;
+  private Map<Path, IjFolder> generatedSourceCodeFoldersMap = new HashMap<>();
   private Map<Path, IjFolder> sourceFoldersMergeMap;
   // See comment in getDependencies for these two member variables.
   private Map<BuildTarget, DependencyType> dependencyTypeMap;
@@ -55,7 +57,6 @@ public ModuleBuildContext(ImmutableSet<BuildTarget> circularDependencyInducingTa
     this.circularDependencyInducingTargets = circularDependencyInducingTargets;
     this.androidFacetBuilder = Optional.empty();
     this.extraClassPathDependenciesBuilder = new ImmutableSet.Builder<>();
-    this.generatedSourceCodeFoldersBuilder = ImmutableSet.builder();
     this.sourceFoldersMergeMap = new HashMap<>();
     this.dependencyTypeMap = new HashMap<>();
     this.dependencyOriginMap = HashMultimap.create();
@@ -83,8 +84,8 @@ public Optional<IjModuleAndroidFacet> getAndroidFacet() {
     return androidFacetBuilder.map(IjModuleAndroidFacet.Builder::build);
   }
 
-  public ImmutableSet<IjFolder> getSourceFolders() {
-    return ImmutableSet.copyOf(sourceFoldersMergeMap.values());
+  public ImmutableCollection<IjFolder> getSourceFolders() {
+    return ImmutableList.copyOf(sourceFoldersMergeMap.values());
   }
 
   public void addExtraClassPathDependency(Path path) {
@@ -96,11 +97,12 @@ public ImmutableSet<Path> getExtraClassPathDependencies() {
   }
 
   public void addGeneratedSourceCodeFolder(IjFolder generatedFolder) {
-    generatedSourceCodeFoldersBuilder.add(generatedFolder);
+    Preconditions.checkState(
+        generatedSourceCodeFoldersMap.put(generatedFolder.getPath(), generatedFolder) == null);
   }
 
-  public ImmutableSet<IjFolder> getGeneratedSourceCodeFolders() {
-    return generatedSourceCodeFoldersBuilder.build();
+  public ImmutableCollection<IjFolder> getGeneratedSourceCodeFolders() {
+    return ImmutableList.copyOf(generatedSourceCodeFoldersMap.values());
   }
 
   public IjModuleType getModuleType() {

src/com/facebook/buck/ide/intellij/model/AbstractContentRoot.java

@@ -18,15 +18,15 @@
 
 import com.facebook.buck.ide.intellij.model.folders.IjSourceFolder;
 import com.facebook.buck.util.immutables.BuckStyleImmutable;
-import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.collect.ImmutableList;
 import org.immutables.value.Value;
 
 @Value.Immutable
 @BuckStyleImmutable
 abstract class AbstractContentRoot implements Comparable<ContentRoot> {
   public abstract String getUrl();
 
-  public abstract ImmutableSortedSet<IjSourceFolder> getFolders();
+  public abstract ImmutableList<IjSourceFolder> getFolders();
 
   @Override
   public int compareTo(ContentRoot o) {

src/com/facebook/buck/ide/intellij/model/AbstractIjModule.java

@@ -23,6 +23,7 @@
 import com.facebook.buck.model.BuildTarget;
 import com.facebook.buck.util.immutables.BuckStyleImmutable;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import java.nio.file.Path;
@@ -51,7 +52,7 @@ public String getName() {
   public abstract Path getModuleBasePath();
 
   /** @return paths to various directories the module is responsible for. */
-  public abstract ImmutableSet<IjFolder> getFolders();
+  public abstract ImmutableList<IjFolder> getFolders();
 
   /**
    * @return map of {@link BuildTarget}s the module depends on and information on whether it's a
@@ -65,7 +66,7 @@ public String getName() {
   public abstract ImmutableSet<Path> getExtraClassPathDependencies();
 
   /** @return Folders which contain the generated source code. */
-  public abstract ImmutableSet<IjFolder> getGeneratedSourceCodeFolders();
+  public abstract ImmutableList<IjFolder> getGeneratedSourceCodeFolders();
 
   public abstract Optional<String> getLanguageLevel();
 

src/com/facebook/buck/ide/intellij/model/folders/IjFolder.java

@@ -27,13 +27,11 @@ public abstract class IjFolder implements Comparable<IjFolder> {
   private final Path path;
   private final ImmutableSortedSet<Path> inputs;
   private final boolean wantsPackagePrefix;
-  private final int inputsHash;
 
   IjFolder(Path path, boolean wantsPackagePrefix, ImmutableSortedSet<Path> inputs) {
     this.path = path;
     this.wantsPackagePrefix = wantsPackagePrefix;
     this.inputs = (inputs == null) ? EMPTY_INPUTS : inputs;
-    this.inputsHash = inputs.hashCode();
   }
 
   IjFolder(Path path, boolean wantsPackagePrefix) {
@@ -124,7 +122,9 @@ public boolean equals(Object other) {
 
   @Override
   public int hashCode() {
-    return (getPath().hashCode() << 31) | (getWantsPackagePrefix() ? 0x8000 : 0) | inputsHash;
+    return (getPath().hashCode() << 31)
+        | (getWantsPackagePrefix() ? 0x8000 : 0)
+        | inputs.hashCode();
   }
 
   @Override

test/com/facebook/buck/ide/intellij/DefaultIjModuleFactoryTest.java

@@ -62,6 +62,7 @@
 import com.facebook.buck.rules.TargetNode;
 import com.facebook.buck.testutil.FakeProjectFilesystem;
 import com.facebook.buck.util.MoreCollectors;
+import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
@@ -415,7 +416,7 @@ public void testJavaLibraryInRoot() {
     assertEquals(Paths.get(""), folder.getPath());
   }
 
-  private ImmutableSet<Path> getFolderPaths(ImmutableSet<IjFolder> folders) {
+  private ImmutableSet<Path> getFolderPaths(ImmutableCollection<IjFolder> folders) {
     return folders.stream().map(IjFolder::getPath).collect(MoreCollectors.toImmutableSet());
   }
 

test/com/facebook/buck/ide/intellij/IjProjectDataPreparerTest.java

@@ -48,6 +48,7 @@
 import com.google.common.base.Functions;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.FluentIterable;
+import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
@@ -96,7 +97,7 @@ public void testWriteModule() throws Exception {
     ContentRoot contentRoot = dataPreparer.getContentRoot(baseModule);
     assertEquals("file://$MODULE_DIR$", contentRoot.getUrl());
 
-    IjSourceFolder baseSourceFolder = contentRoot.getFolders().first();
+    IjSourceFolder baseSourceFolder = contentRoot.getFolders().iterator().next();
     assertEquals("sourceFolder", baseSourceFolder.getType());
     assertFalse(baseSourceFolder.getIsTestSource());
     assertEquals("com.example.base", baseSourceFolder.getPackagePrefix());
@@ -433,7 +434,7 @@ public void testCreatePackageLookupPahtSet() {
         containsInAnyOrder(subSourcePath, sourcePath));
   }
 
-  private static ImmutableSet<Path> distillExcludeFolders(ImmutableSet<IjFolder> folders) {
+  private static ImmutableSet<Path> distillExcludeFolders(ImmutableCollection<IjFolder> folders) {
     Preconditions.checkArgument(
         !FluentIterable.from(folders).anyMatch(input -> !(input instanceof ExcludeFolder)));
     return FluentIterable.from(folders).uniqueIndex(IjFolder::getPath).keySet();