[MJARSIGNER-72] Parallel signing for increased speed #18
Conversation
| * @return a List of File objects | ||
| * @throws MojoExecutionException if it was not possible to build a list of jar files. | ||
| */ | ||
| private List<File> findJarfiles() throws MojoExecutionException { |
There was a problem hiding this comment.
Once again the diff on GitHub displays like I have changed the entire function. View the diff from an IDE to easier see what rows I have only re-indented.
|
I tested this with a Maven project that signs 129 jar files using the DigiCert Keylocker PKCS 11 HSM software as a service and also using DigiCerts TSA server http://timestamp.digicert.com (that is: 2 network based requests per jar file).
|
src/main/java/org/apache/maven/plugins/jarsigner/AbstractJarsignerMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/jarsigner/AbstractJarsignerMojo.java
Outdated
Show resolved
Hide resolved
| @@ -115,6 +122,17 @@ public class JarsignerSignMojo extends AbstractJarsignerMojo { | |||
| @Parameter(property = "jarsigner.maxRetryDelaySeconds", defaultValue = "0") | |||
| private int maxRetryDelaySeconds; | |||
|
|
|||
| /** | |||
| * How many threads to use (in parallel) when signing jar files. Increases performance when signing multiple jar | |||
There was a problem hiding this comment.
Is there a plausible maximum here?
There was a problem hiding this comment.
I thought about this a bit. In https://github.com/apache/maven-surefire it does not look like they have a limit (for both JUnit and TestNG). The https://github.com/apache/maven-compiler-plugin does not have a similar parameter. I tested to set threadCount to 10000 on my 129 jar-file-project. I only got 129 threads (because of the details in how ThreadPoolExecutor per default behaves).
Since I don't know what the future holds (how fast storage, CPU and how much network latency there may be) I suggest that I do not put a limit on this. In practice the threads are limited by the number of jar files anyway.
There was a problem hiding this comment.
In that case change "How many threads to use (in parallel)" --> "Maximum number of parallel threads to use"
| */ | ||
| @Override | ||
| protected void processArchives(List<File> archives) throws MojoExecutionException { | ||
| ExecutorService executor = Executors.newFixedThreadPool(threadCount); |
There was a problem hiding this comment.
should this be the minimum of threadCount and the number of archives? No reason to spawn ten threads with only 5 files to sign
There was a problem hiding this comment.
Nice observation! I could, but I'm not sure I should, because it would make the code a bit harder to read. In the documentation for https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ThreadPoolExecutor.html there is this text:
When a new task is submitted in method execute(Runnable), and fewer than corePoolSize threads are running, a new thread is created to handle the request, even if other worker threads are idle.
That is, for this specific case the number of threads will in practice be limited to the number of jobs I submit to the thread pool (number of jar files). Due to how Executors.newFixedThreadPool() work you might end up with a few more than needed, if the first jobs terminate very fast before the last has been submitted. But the number of threads will never be more than the amount of submitted jobs.
I have tested and verified that this behavior holds! Using threadCount=10000 and looking at the number of threads in my IDE.
src/test/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojoParallelTest.java
Outdated
Show resolved
Hide resolved
| * control the level of some logging events. | ||
| */ | ||
| @Test | ||
| public void testLoggingVerboseTrue() throws Exception { |
There was a problem hiding this comment.
Personally I prefer not to test logging since I don't consider it part of the contract of a method
There was a problem hiding this comment.
I could remove the logging tests? 🤷 The reason I added tests for so much was to be very sure that my refactor did not change anything unexpected.
src/test/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojoTest.java
Outdated
Show resolved
Hide resolved
schedin
force-pushed
the
MJARSIGNER-72_threadCount
branch
2 times, most recently
from
b25fb56 to
6f59f4b
Compare
src/main/java/org/apache/maven/plugins/jarsigner/AbstractJarsignerMojo.java
Outdated
Show resolved
Hide resolved
|
I’m still waiting for feedback regarding if I should follow the https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html example (under the |
schedin
force-pushed
the
MJARSIGNER-72_threadCount
branch
from
6f59f4b to
5710fb7
Compare
| @@ -115,6 +122,17 @@ public class JarsignerSignMojo extends AbstractJarsignerMojo { | |||
| @Parameter(property = "jarsigner.maxRetryDelaySeconds", defaultValue = "0") | |||
| private int maxRetryDelaySeconds; | |||
|
|
|||
| /** | |||
| * How many threads to use (in parallel) when signing jar files. Increases performance when signing multiple jar | |||
There was a problem hiding this comment.
In that case change "How many threads to use (in parallel)" --> "Maximum number of parallel threads to use"
Adding support for threadCount when signing jar files
schedin
force-pushed
the
MJARSIGNER-72_threadCount
branch
from
5710fb7 to
7972fcd
Compare
|
I fixed the javadoc on |
|
Im not sure how this works, but will there eventually be a 3.1.0 (or whatever) Maven Jarsigner plugin that has this change or is it up to individuals to build it? |
I will release 3.1.0 in a few days Always you can ask on Maven Developer List for release ... https://maven.apache.org/mailing-lists.html |
In this pull request I have implemented https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-72 so that signing of jar files can be done in parallel.
This ticket is the reason I added to many test cases in my previous work of implementing MJARSIGNER-41. In the scope of this ticket I have added new test cases to verify that the threading/parallelism works. I have also added some logging tests (to verify that the logging is correctly handled).