Issue checkstyle#344: add multi-thread support to CheckstyleReportsParser

Author: Pavel Bludov

patch-diff-report-tool/src/main/java/com/github/checkstyle/data/CheckstyleRecord.java

@@ -43,6 +43,26 @@ public final class CheckstyleRecord {
      */
     private static final int CHECK_STRING_LENGTH = 5;
 
+    /**
+     * Constant value for severity level "info".
+     */
+    private static final int SEVERITY_INFO = 0;
+
+    /**
+     * Constant value for severity level "warning".
+     */
+    private static final int SEVERITY_WARNING = 1;
+
+    /**
+     * Constant value for severity level "error".
+     */
+    private static final int SEVERITY_ERROR = 2;
+
+    /**
+     * Constant value for all other severity levels.
+     */
+    private static final int SEVERITY_OTHER = 3;
+
     /**
      * Index of the source.
      */
@@ -180,14 +200,55 @@ public String getSimpleCuttedSourceName() {
      * It is used in a single controlled occasion in the code.
      *
      * @param other
-     *        another ChechstyleRecord instance under comparison
+     *        another CheckstyleRecord instance under comparison
      *        with this instance.
      * @return true if instances are equal.
      */
-    public boolean specificEquals(final CheckstyleRecord other) {
-        return this.line == other.line && this.column == other.column
-                && this.source.equals(other.source)
-                && this.message.equals(other.message);
+    public int specificCompare(final CheckstyleRecord other) {
+        int diff = this.source.compareTo(other.source);
+        if (diff == 0) {
+            diff = Integer.compare(this.line, other.line);
+        }
+        if (diff == 0) {
+            diff = Integer.compare(this.column, other.column);
+        }
+        if (diff == 0) {
+            if (!this.severity.equals(other.severity)) {
+                if (getSeverityLevel(this.severity) < getSeverityLevel(other.severity)) {
+                    diff = -1;
+                }
+                else {
+                    diff = 1;
+                }
+            }
+        }
+        if (diff == 0) {
+            diff = this.message.compareTo(other.message);
+        }
+        return diff;
+    }
+
+    /**
+     * Helper method to compare two records in predefined order by severity.
+     *
+     * @param severity to examine
+     * @return the severity order
+     */
+    private static int getSeverityLevel(String severity) {
+        final int result;
+        if ("info".equals(severity)) {
+            result = SEVERITY_INFO;
+        }
+        else if ("warning".equals(severity)) {
+            result = SEVERITY_WARNING;
+        }
+        else if ("error".equals(severity)) {
+            result = SEVERITY_ERROR;
+        }
+        else {
+            result = SEVERITY_OTHER;
+        }
+        return result;
     }
 
 }

patch-diff-report-tool/src/main/java/com/github/checkstyle/data/DiffReport.java

@@ -21,10 +21,10 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.Comparator;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
-import java.util.TreeMap;
+import java.util.concurrent.ConcurrentSkipListMap;
 
 import com.github.checkstyle.parser.CheckstyleReportsParser;
 
@@ -42,21 +42,21 @@ public final class DiffReport {
      * Container for parsed data,
      * note it is a TreeMap for memory keeping purposes.
      */
-    private Map<String, List<CheckstyleRecord>> records =
-            new TreeMap<>();
+    private final Map<String, List<CheckstyleRecord>> records =
+            new ConcurrentSkipListMap<>();
 
     /**
      * Container for statistical data.
      */
-    private Statistics statistics = new Statistics();
+    private final Statistics statistics = new Statistics();
 
     /**
      * Getter for data container.
      *
      * @return map containing parsed data.
      */
     public Map<String, List<CheckstyleRecord>> getRecords() {
-        return records;
+        return Collections.unmodifiableMap(records);
     }
 
     public Statistics getStatistics() {
@@ -76,6 +76,7 @@ public Statistics getStatistics() {
     public void addRecords(List<CheckstyleRecord> newRecords,
             String filename) {
         if (!newRecords.isEmpty()) {
+            newRecords.sort(CheckstyleRecord::specificCompare);
             final List<CheckstyleRecord> popped =
                 records.put(filename, newRecords);
             if (popped != null) {
@@ -85,57 +86,84 @@ public void addRecords(List<CheckstyleRecord> newRecords,
                     records.remove(filename);
                 }
                 else {
-                    Collections.sort(diff, new PositionOrderComparator());
                     records.put(filename, diff);
                 }
             }
         }
     }
 
     /**
-     * Creates difference between 2 lists of records.
+     * Creates difference between 2 sorted lists of records.
      *
-     * @param list1
+     * @param firstList
      *        the first list.
-     * @param list2
+     * @param secondList
      *        the second list.
      * @return the difference list.
      */
     private static List<CheckstyleRecord> produceDiff(
-            List<CheckstyleRecord> list1, List<CheckstyleRecord> list2) {
-        final List<CheckstyleRecord> diff = new ArrayList<>();
-        for (CheckstyleRecord rec1 : list1) {
-            if (!isInList(list2, rec1)) {
-                diff.add(rec1);
-            }
+            List<CheckstyleRecord> firstList, List<CheckstyleRecord> secondList) {
+        final List<CheckstyleRecord> result;
+        if (firstList.isEmpty()) {
+            result = secondList;
         }
-        for (CheckstyleRecord rec2 : list2) {
-            if (!isInList(list1, rec2)) {
-                diff.add(rec2);
-            }
+        else if (secondList.isEmpty()) {
+            result = firstList;
+        }
+        else {
+            result = produceDiff(firstList.iterator(), secondList.iterator());
         }
-        return diff;
+        return result;
     }
 
     /**
-     * Compares the record against list of records.
+     * Creates difference between 2 non-empty iterators of records.
      *
-     * @param list
-     *        of records.
-     * @param testedRecord
-     *        the record.
-     * @return true, if has its copy in a list.
+     * @param firstIterator
+     *        the first iterator.
+     * @param secondIterator
+     *        the second iterator.
+     * @return the difference list (always sorted).
      */
-    private static boolean isInList(List<CheckstyleRecord> list,
-            CheckstyleRecord testedRecord) {
-        boolean belongsToList = false;
-        for (CheckstyleRecord checkstyleRecord : list) {
-            if (testedRecord.specificEquals(checkstyleRecord)) {
-                belongsToList = true;
-                break;
+    private static List<CheckstyleRecord> produceDiff(
+            Iterator<CheckstyleRecord> firstIterator, Iterator<CheckstyleRecord> secondIterator) {
+        CheckstyleRecord firstVal = firstIterator.next();
+        CheckstyleRecord secondVal = secondIterator.next();
+        final List<CheckstyleRecord> result = new ArrayList<>();
+        while (true) {
+            final int diff = firstVal.specificCompare(secondVal);
+            if (diff < 0) {
+                result.add(firstVal);
+                if (!firstIterator.hasNext()) {
+                    result.add(secondVal);
+                    break;
+                }
+                firstVal = firstIterator.next();
+            }
+            else if (diff > 0) {
+                result.add(secondVal);
+                if (!secondIterator.hasNext()) {
+                    result.add(firstVal);
+                    break;
+                }
+                secondVal = secondIterator.next();
+            }
+            else {
+                if (!firstIterator.hasNext() || !secondIterator.hasNext()) {
+                    break;
+                }
+                firstVal = firstIterator.next();
+                secondVal = secondIterator.next();
             }
         }
-        return belongsToList;
+        // add tails
+        while (firstIterator.hasNext()) {
+            result.add(firstIterator.next());
+        }
+        while (secondIterator.hasNext()) {
+            result.add(secondIterator.next());
+        }
+        return result;
     }
 
     /**
@@ -160,27 +188,4 @@ public void getDiffStatistics() {
         }
     }
 
-    /**
-     * Comparator used to sort lists of CheckstyleRecord objects
-     * by their position in code.
-     *
-     * @author atta_troll
-     *
-     */
-    private static class PositionOrderComparator
-        implements Comparator<CheckstyleRecord> {
-
-        @Override
-        public int compare(final CheckstyleRecord arg0,
-                final CheckstyleRecord arg1) {
-            final int difference = arg0.getLine() - arg1.getLine();
-            if (difference == 0) {
-                return arg0.getColumn() - arg1.getColumn();
-            }
-            else {
-                return difference;
-            }
-        }
-    }
-
 }

patch-diff-report-tool/src/main/java/com/github/checkstyle/parser/CheckstyleReportsParser.java

@@ -24,6 +24,7 @@
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
+import java.util.concurrent.CompletableFuture;
 
 import javax.xml.stream.XMLEventReader;
 import javax.xml.stream.XMLStreamException;
@@ -139,6 +140,7 @@ public static DiffReport parse(Path baseXml, Path patchXml, int portionSize)
 
     /**
      * Parses portion of the XML report.
+     * Difference generation is performed asynchronously for efficient CPU usage.
      *
      * @param diffReport
      *        container for parsed data.
@@ -157,6 +159,7 @@ private static void parseXmlPortion(DiffReport diffReport,
         int counter = numOfFilenames;
         String filename = null;
         List<CheckstyleRecord> records = null;
+        final List<CompletableFuture<Void>> tasks = new ArrayList<>();
         while (reader.hasNext()) {
             final XMLEvent event = reader.nextEvent();
             if (event.isStartElement()) {
@@ -187,13 +190,20 @@ else if (startElementName.equals(ERROR_TAG)) {
             if (event.isEndElement()) {
                 final EndElement endElement = event.asEndElement();
                 if (endElement.getName().getLocalPart().equals(FILE_TAG)) {
-                    diffReport.addRecords(records, filename);
+                    final List<CheckstyleRecord> currRecords = records;
+                    records = null;
+                    final String currFile = filename;
+                    filename = null;
+                    tasks.add(CompletableFuture.runAsync(() -> {
+                        diffReport.addRecords(currRecords, currFile);
+                    }));
                     if (counter == 0) {
                         break;
                     }
                 }
             }
         }
+        tasks.forEach(CompletableFuture::join);
     }
 
     /**