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

Support saturating the ForkJoinPool via a property #3027

Closed

Conversation

mpkorstanje
Copy link
Contributor

@mpkorstanje mpkorstanje commented Sep 8, 2022

Overview

The JUnit Platform now supports saturating the ForkJoinPool via a property.
Enabling this property will limit the number of concurrently executing tests
will to the configured parallelism even when some are blocked.

For JUnit Jupiter this can be enabled by through the
junit.jupiter.execution.parallel.config.dynamic.saturate and
junit.jupiter.execution.parallel.config.fixed.saturate configuration
properties.

Fixes: #2545
Fixes: #1858
Fixes: #3026


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

@mpkorstanje mpkorstanje force-pushed the saturate-via-property branch from a29be1f to 9aaf314 Compare September 9, 2022 16:05
@mpkorstanje mpkorstanje changed the title Support saturating the Forkjoin pool via a property Support saturating the ForkJoinPool via a property Sep 9, 2022
@mpkorstanje mpkorstanje force-pushed the saturate-via-property branch 5 times, most recently from 15b660d to dd16846 Compare September 9, 2022 16:20
@mpkorstanje mpkorstanje marked this pull request as ready for review September 9, 2022 16:23
The JUnit Platform now supports saturating the ForkJoinPool via a property.
Enabling this property will limit the number of concurrently executing tests
will to the configured parallelism even when some are blocked.

For JUnit Jupiter this can be enabled by through the
`junit.jupiter.execution.parallel.config.dynamic.saturate` and
`junit.jupiter.execution.parallel.config.fixed.saturate` configuration
properties.

Fixes: junit-team#2545
Fixes: junit-team#1858
Fixes: junit-team#3026
@mpkorstanje mpkorstanje force-pushed the saturate-via-property branch from dd16846 to 2841296 Compare September 9, 2022 16:25
@mpkorstanje
Copy link
Contributor Author

The windows build fails with java.lang.StackOverflowError (no error message) but I've got no way to reproduce this.

@marcphilipp marcphilipp self-requested a review September 16, 2022 10:18
@marcphilipp
Copy link
Member

Fixes: #2545
Fixes: #1858
Fixes: #3026

@mpkorstanje Could you please briefly explain why all three issues would be fixed by merging this PR?

@mpkorstanje
Copy link
Contributor Author

The solution to #1858 and #2545 comes from ensuring the number of running tests does not exceed the desired parallelism. This can be achieved by implementing a strategy that returns a ParallelExecutionConfiguration with saturate = () -> true and parallelism = maxPoolSize = <desired parallelism>.

Unfortunately I overlooked that both the fixed and dynamic strategy adds an additional 256 threads to the maximum pool size, so merely allowing the pool to be saturated doesn't solve the problem.

return new DefaultParallelExecutionConfiguration(parallelism, parallelism, 256 + parallelism, parallelism,
KEEP_ALIVE_SECONDS);

Adding a *.max-pool-size property would resolve that.

@mpkorstanje
Copy link
Contributor Author

On second thought, only fixed.saturate and fixed.max-pool-size make sense. I can't think of any benefits the dynamic strategy would get.

@mpkorstanje mpkorstanje marked this pull request as draft September 16, 2022 15:19
@mpkorstanje
Copy link
Contributor Author

mpkorstanje commented Sep 16, 2022

And on third thought, with the fixed strategy is there a reason not to allow the fork join pool to be saturated. Tests rejected by the pool would fail when the pool could not be saturated, but what value would that have?

@mpkorstanje
Copy link
Contributor Author

I'll come back to this when I've got a coherent proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants