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

pbludov · 2020-10-31T17:27:10Z

Issue #344

@rnveach @romani @strkkk
Here is my attempt to speed up the diff report generation

CheckstyleRecord now implements Comparable
The method specificEquals now Comparable::compareTo as we need the order of records
The list of records to process is pre-sorted (it has to be already sorted, so it does not take much time)
The method produceDiff now is O(N) as both lists are sorted
Difference generation is threaded to utilize more CPU and speed up the process
PositionOrderComparator removed
produceDiff now is a generic method and extracted to DiffUtils class
CliPaths renamed to CliOptions
New option for threading mode: SINGLE or MULTI

TODO:

Do some regression for regression testing
Add an option to disable multithreading to easily diagnose any problems with threading that may occur in the future

pbludov · 2020-10-31T18:48:02Z

Running diff on openjdk14 with modules

<module name="Indentation"/>
<module name="MagicNumber"/>
<module name="MissingCtor"/>
<module name="RequireThis"/>
<module name="ReturnCount">
  <property name="max" value="1"/>
  <property name="maxForVoid" value="0"/>
</module>
<module name="SimplifyBooleanExpression"/>
<module name="SimplifyBooleanReturn"/>
<module name="StringLiteralEquality"/>
<module name="VariableDeclarationUsageDistance"/>
<module name="AvoidStarImport"/>
<module name="AvoidStaticImport"/>
<module name="MissingJavadocMethod">
  <property name="allowMissingPropertyJavadoc" value="true"/>
</module>
<module name="MissingJavadocPackage"/>
<module name="MissingJavadocType"/>
<module name="JavaNCSS"/>
<module name="NPathComplexity"/>

produces checkstyle-result.xml with size 140M

Diff generation timings (best result from 10 runs)

Master:

real    0m13,796s
user    0m16,818s
sys     0m0,251s

Patch:

real    0m4,518s
user    0m8,289s
sys     0m0,409s

romani · 2020-10-31T18:57:30Z

Heh, from 13 second to 4 seconds, in worse case.
If we merge this , it will be only to please maintainers to put MT in code.

pbludov · 2020-10-31T19:07:17Z

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

@@ -76,6 +77,7 @@ public Statistics getStatistics() {
    public void addRecords(List<CheckstyleRecord> newRecords,
            String filename) {
        if (!newRecords.isEmpty()) {
+            newRecords.sort(new PositionOrderComparator());


Why we are using PositionOrderComparator for sorting lines within a file?
This comparator uses only line and column, as a result there may be false positives for

line 100, col 100, messageA line 100, col 100, messageB

versus

line 100, col 100, messageB line 100, col 100, messageA

I'm going to use same specificCompare method here

We did not review this code thoroughly, it is just a code that works

Done. Well, now i wonder why CheckstyleRecord does not implement Comparable but instead exposes a method used in sorting? This should be reviewed.

Just fix it if you think it weird

rnveach · 2020-11-01T00:32:14Z

If we merge this , it will be only to please maintainers to put MT in code.

It really depends on the differences. When I created the issue, I had a 1 hour run and MT reduced it to 20 minutes.

rnveach · 2020-11-01T00:39:26Z

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

+ * Severity level for a {@link CheckstyleRecord}.
+ * Used to compare two records in predefined order: "info", "warning", "error", all other.
+ */
+public enum SeverityLevel {


Why not https://github.com/checkstyle/checkstyle/blob/2e8c277c181487d81231170ff4b959edc008114e/src/main/java/com/puppycrawl/tools/checkstyle/api/SeverityLevel.java#L34 ?

Diff report tool has no dependency on Checkstyle itself. This enum is to provide an order other then alphabetical to record severity. May be this enum is redundant and confusing. I'm going to review it.

done. @rnveach please review

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

pbludov · 2020-11-01T09:31:10Z

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

-                statistics.incrementUniqueMessageCount(rec.getIndex());
-            }
-        }
+        records.entrySet().parallelStream()


Parallelizing this loop gives 5% more speed for large diffs.

upd: not in this PR. The class Statistics is not thread-safe.

…rser

pbludov · 2020-11-04T21:00:56Z

The diff report is available at https://pbludov.github.io/

The first report is built with patch-diff-report-tool from master
The second report is from this branch in single-threaded mode
The third report is from this branch in multi-threaded mode

They are all equal.

rnveach · 2020-11-05T21:24:11Z

To be updated as jobs run:

Files used: http://rveach.no-ip.org/checkstyle/regression/reports/294/ (Warning XML is 169 megs) (Patch and master XMLs are 100% identical)
Command:

$ time java -jar ~/opensource/contribution/patch-diff-report-tool/target/patch-diff-report-tool-0.1-SNAPSHOT-jar-with-dependencies.jar --baseReport /var/www/html/reports/294/openjdk-pull.xml --patchReport /var/www/html/reports/294/openjdk-master.xml --output /tmp/test1 --baseConfig /var/www/html/reports/294/config.xml --patchConfig /var/www/html/reports/294/config.xml --refFiles /home/ricky/regression_repositories/openjdk10

master run:

patch-diff-report-tool execution started.
XML parsing is started.
Creation of configuration report is started.
Creation of diff html site is started.
Creation of the result site succeed.
patch-diff-report-tool execution finished.

real	60m10.596s
user	60m22.767s
sys	0m3.359s

this PR's run:

patch-diff-report-tool execution started.
XML parsing is started.
Creation of configuration report is started.
Creation of diff html site is started.
Creation of the result site succeed.
patch-diff-report-tool execution finished.

real	0m12.387s
user	0m23.788s
sys	0m1.658s

my PR's run with 6 processors:

patch-diff-report-tool execution started.
XML parsing is started.
Creation of configuration report is started.
Creation of diff html site is started.
Creation of the result site succeed.
patch-diff-report-tool execution finished.

real	21m45.254s
user	128m24.969s
sys	0m29.677s

rnveach · 2020-11-05T23:19:51Z

@pbludov I do like your changes more. The thing is your changes aren't multi-threading. It is just pure optimization and so it all still runs single threaded. The quick run is just from 2 exact same files, I would like to see what is the time when everything is different. I assume it would then result in O(N^2) time since we have to go through each list twice, plus adding a sorting step which may result in longer times the more random the data gets.

I am fine if we want to keep this but still look into some way to multi-thread this.

rnveach · 2020-11-21T03:23:04Z

@pbludov @romani Any thoughts?

I will try to find time to create a fully random files to test.

pbludov · 2020-11-21T14:39:50Z

@pbludov @romani Any thoughts?

I also noticed that the only 1.2 cores are loaded (on my PC). The main code is still single-threaded and has many ways for parallelization. But we can't go any further until we have a reliable method of regression testing.

pbludov · 2020-12-27T11:04:06Z

@rnveach This takes much more time than I expected. Let's merge the optimization part and do multithreading later.

#533

pbludov requested review from rnveach, romani and strkkk October 31, 2020 17:27

pbludov mentioned this pull request Oct 31, 2020

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

Closed

pbludov force-pushed the issue-344-threading branch from 876c2b1 to 5c67b71 Compare October 31, 2020 18:31

romani assigned rnveach Oct 31, 2020

pbludov commented Oct 31, 2020

View reviewed changes

pbludov force-pushed the issue-344-threading branch from 5c67b71 to 1b3ea10 Compare October 31, 2020 19:30

pbludov marked this pull request as draft October 31, 2020 19:35

pbludov force-pushed the issue-344-threading branch from 1b3ea10 to 9889f52 Compare October 31, 2020 19:57

pbludov marked this pull request as ready for review October 31, 2020 21:02

pbludov force-pushed the issue-344-threading branch from 9889f52 to 5c70a4b Compare October 31, 2020 21:52

rnveach reviewed Nov 1, 2020

View reviewed changes

patch-diff-report-tool/src/main/java/com/github/checkstyle/data/DiffReport.java Outdated Show resolved Hide resolved

pbludov force-pushed the issue-344-threading branch from 5c70a4b to 611ef49 Compare November 1, 2020 09:26

pbludov commented Nov 1, 2020

View reviewed changes

pbludov force-pushed the issue-344-threading branch 3 times, most recently from fd8f6ba to b65b85a Compare November 4, 2020 20:45

Issue checkstyle#344: add multi-thread support to CheckstyleReportsPa…

f48cea1

…rser

pbludov force-pushed the issue-344-threading branch from b65b85a to f48cea1 Compare November 4, 2020 20:58

pbludov requested a review from rnveach November 4, 2020 21:21

rnveach assigned pbludov and unassigned rnveach Nov 14, 2020

This was referenced Dec 27, 2020

Optimize DiffReport generation #532

Closed

Issue #532: optimize DiffReport #533

Merged

pbludov marked this pull request as draft December 27, 2020 11:04

pbludov closed this Jan 30, 2021

pbludov deleted the issue-344-threading branch January 30, 2021 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pbludov commented Oct 31, 2020 •

edited

Loading

pbludov commented Oct 31, 2020

romani commented Oct 31, 2020

pbludov Oct 31, 2020

romani Oct 31, 2020

pbludov Oct 31, 2020

romani Oct 31, 2020

pbludov Oct 31, 2020

rnveach commented Nov 1, 2020

rnveach Nov 1, 2020

pbludov Nov 1, 2020

pbludov Nov 4, 2020

pbludov Nov 1, 2020

pbludov Nov 1, 2020

pbludov commented Nov 4, 2020

rnveach commented Nov 5, 2020 •

edited

Loading

rnveach commented Nov 5, 2020

rnveach commented Nov 21, 2020 •

edited

Loading

pbludov commented Nov 21, 2020 •

edited

Loading

pbludov commented Dec 27, 2020

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

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

Conversation

pbludov commented Oct 31, 2020 • edited Loading

pbludov commented Oct 31, 2020

Diff generation timings (best result from 10 runs)

romani commented Oct 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach commented Nov 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbludov commented Nov 4, 2020

rnveach commented Nov 5, 2020 • edited Loading

rnveach commented Nov 5, 2020

rnveach commented Nov 21, 2020 • edited Loading

pbludov commented Nov 21, 2020 • edited Loading

pbludov commented Dec 27, 2020

pbludov commented Oct 31, 2020 •

edited

Loading

rnveach commented Nov 5, 2020 •

edited

Loading

rnveach commented Nov 21, 2020 •

edited

Loading

pbludov commented Nov 21, 2020 •

edited

Loading