SONAR-5878 High memory consumption for very big Maven projects

7 files changed, 138 insertions, 88 deletions

diff --git a/sonar-batch/src/main/java/org/sonar/batch/scan/DeprecatedProjectReactorBuilder.java b/sonar-batch/src/main/java/org/sonar/batch/scan/DeprecatedProjectReactorBuilder.java
index c9983e33243..7d04d8e24d0 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/scan/DeprecatedProjectReactorBuilder.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/scan/DeprecatedProjectReactorBuilder.java
@@ -71,7 +71,7 @@ public class DeprecatedProjectReactorBuilder extends ProjectReactorBuilder {
 
     mergeParentProperties(moduleProps, parentProject.getProperties());
 
-    return defineProject(moduleProps, parentProject);
+    return defineRootProject(moduleProps, parentProject);
   }
 
   /**
diff --git a/sonar-batch/src/main/java/org/sonar/batch/scan/ProjectReactorBuilder.java b/sonar-batch/src/main/java/org/sonar/batch/scan/ProjectReactorBuilder.java
index 68f62cf4e98..9c1775d23a8 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/scan/ProjectReactorBuilder.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/scan/ProjectReactorBuilder.java
@@ -43,10 +43,14 @@ import java.io.FileInputStream;
 import java.io.IOException;
 import java.text.MessageFormat;
 import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Properties;
+import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeSet;
 
 /**
  * Class that creates a project definition based on a set of properties.
@@ -101,48 +105,97 @@ public class ProjectReactorBuilder {
   private static final List<String> NON_HERITED_PROPERTIES_FOR_CHILD = Lists.newArrayList(PROPERTY_PROJECT_BASEDIR, CoreProperties.WORKING_DIRECTORY, PROPERTY_MODULES,
     CoreProperties.PROJECT_DESCRIPTION_PROPERTY);
 
-  private TaskProperties props;
+  private TaskProperties taskProps;
   private File rootProjectWorkDir;
 
   public ProjectReactorBuilder(TaskProperties props) {
-    this.props = props;
+    this.taskProps = props;
   }
 
   public ProjectReactor execute() {
-    Properties bootstrapProperties = new Properties();
-    bootstrapProperties.putAll(props.properties());
-    ProjectDefinition rootProject = defineProject(bootstrapProperties, null);
+    Properties allProperties = new Properties();
+    allProperties.putAll(taskProps.properties());
+    SortedSet<String> moduleIds = new TreeSet<String>(new Comparator<String>() {
+      @Override
+      public int compare(String o1, String o2) {
+        // Reverse string order
+        return o2.compareTo(o1);
+      }
+    });
+    collectModuleIds(null, allProperties, moduleIds);
+    Map<String, Properties> propertiesByModuleId = extractPropertiesByModule(allProperties, new ArrayList<String>(moduleIds));
+    ProjectDefinition rootProject = defineRootProject(propertiesByModuleId.get(""), null);
     rootProjectWorkDir = rootProject.getWorkDir();
-    defineChildren(rootProject);
+    defineChildren(rootProject, propertiesByModuleId);
     cleanAndCheckProjectDefinitions(rootProject);
+    // Optimization now that all properties have been read and stored in appropriate ProjectDefinition
+    taskProps.properties().clear();
     return new ProjectReactor(rootProject);
   }
 
-  protected ProjectDefinition defineProject(Properties properties, @Nullable ProjectDefinition parent) {
-    if (properties.containsKey(PROPERTY_MODULES)) {
-      checkMandatoryProperties(properties, MANDATORY_PROPERTIES_FOR_MULTIMODULE_PROJECT);
+  private Map<String, Properties> extractPropertiesByModule(Properties allProperties, List<String> moduleIds) {
+    Map<String, Properties> result = new HashMap<String, Properties>();
+    for (String moduleId : moduleIds) {
+      result.put(moduleId, new Properties());
+    }
+    // For root module
+    result.put("", new Properties());
+
+    for (Map.Entry<Object, Object> entry : allProperties.entrySet()) {
+      String key = (String) entry.getKey();
+      boolean found = false;
+      for (String moduleId : moduleIds) {
+        String propertyPrefix = moduleId + ".";
+        int prefixLength = propertyPrefix.length();
+        if (key.startsWith(propertyPrefix)) {
+          result.get(moduleId).put(key.substring(prefixLength), entry.getValue());
+          found = true;
+          break;
+        }
+      }
+      if (!found) {
+        result.get("").put(key, entry.getValue());
+      }
+    }
+    return result;
+  }
+
+  private void collectModuleIds(String currentModuleId, Properties allProperties, Set<String> moduleIds) {
+    if (currentModuleId != null) {
+      if (!moduleIds.add(currentModuleId)) {
+        throw new IllegalStateException("Module ID '" + currentModuleId + "' is defined several times in the reactor");
+      }
+    }
+    for (String moduleId : getListFromProperty(allProperties, (currentModuleId == null ? "" : (currentModuleId + ".")) + PROPERTY_MODULES)) {
+      collectModuleIds(moduleId, allProperties, moduleIds);
+    }
+  }
+
+  protected ProjectDefinition defineRootProject(Properties rootProperties, @Nullable ProjectDefinition parent) {
+    if (rootProperties.containsKey(PROPERTY_MODULES)) {
+      checkMandatoryProperties(rootProperties, MANDATORY_PROPERTIES_FOR_MULTIMODULE_PROJECT);
     } else {
-      checkMandatoryProperties(properties, MANDATORY_PROPERTIES_FOR_SIMPLE_PROJECT);
+      checkMandatoryProperties(rootProperties, MANDATORY_PROPERTIES_FOR_SIMPLE_PROJECT);
     }
-    File baseDir = new File(properties.getProperty(PROPERTY_PROJECT_BASEDIR));
-    final String projectKey = properties.getProperty(CoreProperties.PROJECT_KEY_PROPERTY);
+    File baseDir = new File(rootProperties.getProperty(PROPERTY_PROJECT_BASEDIR));
+    final String projectKey = rootProperties.getProperty(CoreProperties.PROJECT_KEY_PROPERTY);
     File workDir;
     if (parent == null) {
-      validateDirectories(properties, baseDir, projectKey);
+      validateDirectories(rootProperties, baseDir, projectKey);
       workDir = initRootProjectWorkDir(baseDir);
     } else {
-      workDir = initModuleWorkDir(baseDir, properties);
+      workDir = initModuleWorkDir(baseDir, rootProperties);
     }
 
-    return ProjectDefinition.create().setProperties(properties)
+    return ProjectDefinition.create().setProperties(rootProperties)
       .setBaseDir(baseDir)
       .setWorkDir(workDir)
-      .setBuildDir(initModuleBuildDir(baseDir, properties));
+      .setBuildDir(initModuleBuildDir(baseDir, rootProperties));
   }
 
   @VisibleForTesting
   protected File initRootProjectWorkDir(File baseDir) {
-    String workDir = props.property(CoreProperties.WORKING_DIRECTORY);
+    String workDir = taskProps.property(CoreProperties.WORKING_DIRECTORY);
     if (StringUtils.isBlank(workDir)) {
       return new File(baseDir, CoreProperties.WORKING_DIRECTORY_DEFAULT_VALUE);
     }
@@ -184,16 +237,16 @@ public class ProjectReactorBuilder {
     return new File(moduleBaseDir, customBuildDir.getPath());
   }
 
-  private void defineChildren(ProjectDefinition parentProject) {
+  private void defineChildren(ProjectDefinition parentProject, Map<String, Properties> propertiesByModuleId) {
     Properties parentProps = parentProject.getProperties();
     if (parentProps.containsKey(PROPERTY_MODULES)) {
-      for (String module : getListFromProperty(parentProps, PROPERTY_MODULES)) {
-        Properties moduleProps = extractModuleProperties(module, parentProps);
-        ProjectDefinition childProject = loadChildProject(parentProject, moduleProps, module);
+      for (String moduleId : getListFromProperty(parentProps, PROPERTY_MODULES)) {
+        Properties moduleProps = propertiesByModuleId.get(moduleId);
+        ProjectDefinition childProject = loadChildProject(parentProject, moduleProps, moduleId);
         // check the uniqueness of the child key
         checkUniquenessOfChildKey(childProject, parentProject);
         // the child project may have children as well
-        defineChildren(childProject);
+        defineChildren(childProject, propertiesByModuleId);
         // and finally add this child project to its parent
         parentProject.addSubProject(childProject);
       }
@@ -218,7 +271,7 @@ public class ProjectReactorBuilder {
 
     mergeParentProperties(moduleProps, parentProject.getProperties());
 
-    return defineProject(moduleProps, parentProject);
+    return defineRootProject(moduleProps, parentProject);
   }
 
   @VisibleForTesting
@@ -366,54 +419,19 @@ public class ProjectReactorBuilder {
     properties.remove(PROPERTY_TESTS);
     properties.remove(PROPERTY_BINARIES);
     properties.remove(PROPERTY_LIBRARIES);
-
-    // and they don't need properties related to their modules either
-    Properties clone = (Properties) properties.clone();
-    List<String> moduleIds = Lists.newArrayList(getListFromProperty(properties, PROPERTY_MODULES));
-    for (Entry<Object, Object> entry : clone.entrySet()) {
-      String key = (String) entry.getKey();
-      if (isKeyPrefixedByModuleId(key, moduleIds)) {
-        properties.remove(key);
-      }
-    }
   }
 
   @VisibleForTesting
   protected static void mergeParentProperties(Properties childProps, Properties parentProps) {
-    List<String> moduleIds = Lists.newArrayList(getListFromProperty(parentProps, PROPERTY_MODULES));
     for (Map.Entry<Object, Object> entry : parentProps.entrySet()) {
       String key = (String) entry.getKey();
-      if (!childProps.containsKey(key)
-        && !NON_HERITED_PROPERTIES_FOR_CHILD.contains(key)
-        && !isKeyPrefixedByModuleId(key, moduleIds)) {
+      if ((!childProps.containsKey(key) || childProps.get(key).equals(entry.getValue()))
+        && !NON_HERITED_PROPERTIES_FOR_CHILD.contains(key)) {
         childProps.put(entry.getKey(), entry.getValue());
       }
     }
   }
 
-  private static boolean isKeyPrefixedByModuleId(String key, List<String> moduleIds) {
-    for (String moduleId : moduleIds) {
-      if (key.startsWith(moduleId + ".")) {
-        return true;
-      }
-    }
-    return false;
-  }
-
-  @VisibleForTesting
-  protected static Properties extractModuleProperties(String module, Properties properties) {
-    Properties moduleProps = new Properties();
-    String propertyPrefix = module + ".";
-    int prefixLength = propertyPrefix.length();
-    for (Map.Entry<Object, Object> entry : properties.entrySet()) {
-      String key = (String) entry.getKey();
-      if (key.startsWith(propertyPrefix)) {
-        moduleProps.put(key.substring(prefixLength), entry.getValue());
-      }
-    }
-    return moduleProps;
-  }
-
   @VisibleForTesting
   protected static void checkExistenceOfDirectories(String moduleRef, File baseDir, String[] dirPaths, String propName) {
     for (String path : dirPaths) {
diff --git a/sonar-batch/src/test/java/org/sonar/batch/scan/ProjectReactorBuilderTest.java b/sonar-batch/src/test/java/org/sonar/batch/scan/ProjectReactorBuilderTest.java
index 631a58c766e..64918c83da3 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/scan/ProjectReactorBuilderTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/scan/ProjectReactorBuilderTest.java
@@ -289,25 +289,37 @@ public class ProjectReactorBuilderTest {
   }
 
   @Test
-  public void shouldExtractModuleProperties() {
-    Properties props = new Properties();
-    props.setProperty("sources", "src/main/java");
-    props.setProperty("tests", "src/test/java");
-    props.setProperty("foo.sources", "src/main/java");
-    props.setProperty("foobar.tests", "src/test/java");
-    props.setProperty("foobar.binaries", "target/classes");
-
-    Properties moduleProps = ProjectReactorBuilder.extractModuleProperties("bar", props);
-    assertThat(moduleProps.size()).isEqualTo(0);
-
-    moduleProps = ProjectReactorBuilder.extractModuleProperties("foo", props);
-    assertThat(moduleProps.size()).isEqualTo(1);
-    assertThat(moduleProps.get("sources")).isEqualTo("src/main/java");
-
-    moduleProps = ProjectReactorBuilder.extractModuleProperties("foobar", props);
-    assertThat(moduleProps.size()).isEqualTo(2);
-    assertThat(moduleProps.get("tests")).isEqualTo("src/test/java");
-    assertThat(moduleProps.get("binaries")).isEqualTo("target/classes");
+  public void modulePropertiesShouldNotLeak() throws IOException {
+    ProjectDefinition projectDefinition = loadProjectDefinition("big-multi-module-definitions-all-in-root");
+
+    assertThat(projectDefinition.getProperties().getProperty("module11.property")).isNull();
+    ProjectDefinition module1 = null;
+    ProjectDefinition module2 = null;
+    for (ProjectDefinition prj : projectDefinition.getSubProjects()) {
+      if (prj.getKey().equals("com.foo.project:module1")) {
+        module1 = prj;
+      } else if (prj.getKey().equals("com.foo.project:module2")) {
+        module2 = prj;
+      }
+    }
+    assertThat(module1.getProperties().getProperty("module11.property")).isNull();
+    assertThat(module1.getProperties().getProperty("property")).isNull();
+    assertThat(module2.getProperties().getProperty("module11.property")).isNull();
+    assertThat(module2.getProperties().getProperty("property")).isNull();
+
+    ProjectDefinition module11 = null;
+    ProjectDefinition module12 = null;
+    for (ProjectDefinition prj : module1.getSubProjects()) {
+      if (prj.getKey().equals("com.foo.project:module1:module11")) {
+        module11 = prj;
+      } else if (prj.getKey().equals("com.foo.project:module1:module12")) {
+        module12 = prj;
+      }
+    }
+    assertThat(module11.getProperties().getProperty("module11.property")).isNull();
+    assertThat(module11.getProperties().getProperty("property")).isEqualTo("My module11 property");
+    assertThat(module12.getProperties().getProperty("module11.property")).isNull();
+    assertThat(module12.getProperties().getProperty("property")).isNull();
   }
 
   @Test
@@ -377,28 +389,29 @@ public class ProjectReactorBuilderTest {
 
   @Test
   public void shouldMergeParentProperties() {
+    // Use a random value to avoid VM optimization that would create constant String and make s1 and s2 the same object
+    int i = (int) Math.random() * 10;
+    String s1 = "value" + i;
+    String s2 = "value" + i;
     Properties parentProps = new Properties();
     parentProps.setProperty("toBeMergeProps", "fooParent");
     parentProps.setProperty("existingChildProp", "barParent");
-    parentProps.setProperty("sonar.modules", "mod1,mod2");
+    parentProps.setProperty("duplicatedProp", s1);
     parentProps.setProperty("sonar.projectDescription", "Desc from Parent");
-    parentProps.setProperty("mod1.sonar.projectDescription", "Desc for Mod1");
-    parentProps.setProperty("mod2.sonar.projectkey", "Key for Mod2");
 
     Properties childProps = new Properties();
     childProps.setProperty("existingChildProp", "barChild");
     childProps.setProperty("otherProp", "tutuChild");
+    childProps.setProperty("duplicatedProp", s2);
 
     ProjectReactorBuilder.mergeParentProperties(childProps, parentProps);
 
-    assertThat(childProps.size()).isEqualTo(3);
+    assertThat(childProps).hasSize(4);
     assertThat(childProps.getProperty("toBeMergeProps")).isEqualTo("fooParent");
     assertThat(childProps.getProperty("existingChildProp")).isEqualTo("barChild");
     assertThat(childProps.getProperty("otherProp")).isEqualTo("tutuChild");
-    assertThat(childProps.getProperty("sonar.modules")).isNull();
     assertThat(childProps.getProperty("sonar.projectDescription")).isNull();
-    assertThat(childProps.getProperty("mod1.sonar.projectDescription")).isNull();
-    assertThat(childProps.getProperty("mod2.sonar.projectkey")).isNull();
+    assertThat(childProps.getProperty("duplicatedProp")).isSameAs(parentProps.getProperty("duplicatedProp"));
   }
 
   @Test
diff --git a/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/module1/module11/sources/Fake.java b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/module1/module11/sources/Fake.java
new file mode 100644
index 00000000000..b2e6462a3f9
--- /dev/null
+++ b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/module1/module11/sources/Fake.java
@@ -0,0 +1 @@
+Fake
diff --git a/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/module1/module12/sources/Fake.java b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/module1/module12/sources/Fake.java
new file mode 100644
index 00000000000..b2e6462a3f9
--- /dev/null
+++ b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/module1/module12/sources/Fake.java
@@ -0,0 +1 @@
+Fake
diff --git a/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/module2/sources/Fake.java b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/module2/sources/Fake.java
new file mode 100644
index 00000000000..b2e6462a3f9
--- /dev/null
+++ b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/module2/sources/Fake.java
@@ -0,0 +1 @@
+Fake
diff --git a/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/sonar-project.properties b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/sonar-project.properties
new file mode 100644
index 00000000000..bbc6f993ffb
--- /dev/null
+++ b/sonar-batch/src/test/resources/org/sonar/batch/scan/ProjectReactorBuilderTest/big-multi-module-definitions-all-in-root/sonar-project.properties
@@ -0,0 +1,16 @@
+sonar.projectKey=com.foo.project
+sonar.projectName=Foo Project
+sonar.projectVersion=1.0-SNAPSHOT
+sonar.projectDescription=Description of Foo Project
+
+sonar.sources=sources
+sonar.tests=tests
+sonar.binaries=target/classes
+
+sonar.modules=module1,module2
+
+
+module1.sonar.modules=module11,module12
+
+module11.property=My module11 property
+