Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make SAT thread-safe, improve performance and update SpotBugs #365

Merged
merged 8 commits into from
Jan 17, 2020

Conversation

wborn
Copy link
Member

@wborn wborn commented Nov 18, 2019

This PR makes the SAT Maven plugins thread-safe when used in parallel Maven builds.

Fixes the following theading issues:

  • SpotBugs is only thread-safe when it uses the forking mode which is enabled by default but disabled in the SAT configuration.
    SpotBugs uses Apache Commons BCEL as dependency which is not thread-safe. Forking the SpotBugs execution works around this issue.
    Without forking enabled parallel builds using SpotBugs will also lock up (see Lockup in maven concurrent build, with fork=false spotbugs/spotbugs-maven-plugin#104).
  • Use JVM singleton merge/summary locks for synchronizing the SAT summary report updates.
    Because multiple classloaders are used in Maven builds, it's not possible to use a static lock for synchronization in the ReportUtility Mojo.

The Mojos have also been marked as threadSafe so Maven no longer shows warnings when they are executed in parallel.

For the SpotBugs updates see:

The errors resulting from the findbugs-slf4j upgrade have already been fixed by:

Performance has also been improved considerably by moving the HTML summary generation to an extension.
This way SAT can periodically update the HTML summary instead of continuously.
Updating the HTML summaries can take 20 seconds or more when there are many findings.

Fixes #200


These changes speed up both single threaded and parallel builds using SAT considerably!

Below are some openhab2-addons build results on my quad core i7-6700K (no tests because they fail, openhab/openhab2-addons#6408):

SAT version Command Duration Speedup
0.8.0 mvn clean install -DskipTests 44:33 min 1.00x
0.9.0-SNAPSHOT mvn clean install -DskipTests 23:05 min 1.93x
0.9.0-SNAPSHOT mvn clean install -DskipTests -T 1.5C 13:31 min 3.29x

The parallel builds max out all CPUs. I can only imagine how fast a parallel build will be on a machine with 16 cores. :-)

The extension should be added to the <build> section in POMs, see wborn/openhab-addons@fe73013.

@wborn wborn force-pushed the sat-thread-safe branch 4 times, most recently from b24fa9b to 0add602 Compare November 18, 2019 22:37
@wborn wborn marked this pull request as ready for review November 18, 2019 22:42
@wborn wborn requested a review from a team November 18, 2019 22:42
@martinvw
Copy link
Member

Nice!

@wborn
Copy link
Member Author

wborn commented Dec 22, 2019

It would be awesome to reduce our build times by having this in a new SAT release @openhab/sat-maintainers. 😜

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just 2 small comments.

sat-extension/pom.xml Outdated Show resolved Hide resolved
@wborn
Copy link
Member Author

wborn commented Dec 23, 2019

Also I just noticed there are still some threading issues on Java 8 so I'll look into that too. They aren't there on Java 11 to which I try to default these days. Sometimes I switch back for other projects and then end up testing openHAB code with Java 8 too.

@Hilbrand
Copy link
Member

Do you still want to look into the java8 issues or can I merge this?

I can only imagine how fast a parallel build will be on a machine with 16 cores. :-)

Darn. Now I got distracted browsing for a new computer 😄

@wborn
Copy link
Member Author

wborn commented Dec 23, 2019

I am still looking into how well this works for Java 8.

Yes I'm also feeling my CPU is getting dated. But they are adding more cores every year so I'll just hold on for it a bit longer to get a better deal. ;-)

@kaikreuzer
Copy link
Member

It would be awesome to reduce our build times by having this in a new SAT release

Absolutely! I just didn't react on it before the release as I didn't want to risk anything ;-)
But now it is definitely time to benefit from this great work!

I am still looking into how well this works for Java 8.

Why? In the light of openhab/openhab-core#1287, I think it would be ok if we only support Java 11 (and keep the 2.5.x build on the current SAT version).

@wborn
Copy link
Member Author

wborn commented Dec 23, 2019

Apparently these FileChannel locks can be buggy on Java 8 and behave differently between OS-es.
So I did some more searching and found this Stack Overflow post.

It's possible get a singleton over different class loaders by always loading a class using the same class loader and a lot of reflection. I tried that and it didn't work well and all the reflection didn't make it nice code.

The other option is by just adding the singleton to the System properties. It may not be ideal but it does work on Java 8 and Java 11 and it results in less and nicer code.

This PR makes the SAT Maven plugins thread-safe when used in parallel Maven builds.

Fixes the following theading issues:

* SpotBugs is only thread-safe when it uses the forking mode which is enabled by default but disabled in the SAT configuration.
  SpotBugs uses Apache Commons BCEL as dependency which is not thread-safe. Forking the SpotBugs execution works around this issue.
  Without forking enabled parallel builds using SpotBugs will also lock up (see spotbugs/spotbugs-maven-plugin#104).
* Use lock files for synchronizing the SAT summary report updates.
  Because multiple classloaders are used in Maven builds, it's not possible to use a static lock for synchronization in the ReportUtility Mojo.

The Mojos have also been marked as threadSafe so Maven no longer shows warnings when they are executed in parallel.

For the SpotBugs updates see:

* Spotbugs 3.1.12 release notes:
  https://github.com/spotbugs/spotbugs/blob/3.1.12/CHANGELOG.md#3112---2019-02-28
* findbugs-slf4j 1.5.0 release notes:
  https://github.com/KengoTODA/findbugs-slf4j/blob/master/CHANGELOG.md#150---2019-07-04

The errors resulting from the findbugs-slf4j upgrade have already been fixed by:

* openhab/openhab-core#1213
* openhab/openhab2-addons#6394

Performance has also been improved considerably by moving the HTML summary generation to an extension.
This way SAT can periodically update the HTML summary instead of continuously.
Updating the HTML summaries can take 20 seconds or more when there are many findings.

Fixes openhab#200

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Wouter Born <github@maindrain.net>
@kaikreuzer kaikreuzer merged commit a7d31f6 into openhab:master Jan 17, 2020
@kaikreuzer
Copy link
Member

Awesome work, @wborn!

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Jan 17, 2020
@kaikreuzer
Copy link
Member

@wborn
Copy link
Member Author

wborn commented Jan 17, 2020

Thanks for making the new release! 👍

@wborn wborn deleted the sat-thread-safe branch January 17, 2020 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parallel builds using static-code-analysis get stuck and fail
4 participants