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 creating ForkJoinPools with maximumPoolSize #483

Closed
pjfanning opened this issue Jul 14, 2023 · 13 comments
Closed

support creating ForkJoinPools with maximumPoolSize #483

pjfanning opened this issue Jul 14, 2023 · 13 comments
Milestone

Comments

@pjfanning
Copy link
Contributor

pjfanning commented Jul 14, 2023

ForkJoinPool has extra contructors in JDK9+ that allow maximumPoolSize to be set.

Relates to #482

Pekko still supports JDK8 so implementing this is not straightforward.

Options include:

  • adding a new dispatcher executor type of fork-join-executor-jdk9 which is similar to fork-join-executor but supports the extra maximumPoolSize config (the class for this could be added to jdk9+ build (we support having a scala-jdk9 source directory)
  • have fork-join-executor support the maximumPoolSize but ignore it or fail if Java 8 runtime is used. We might need to use Java reflection to do this.
@pjfanning pjfanning added this to the 1.1.0 milestone Jul 17, 2023
@He-Pin He-Pin modified the milestones: 1.1.0, 1.1.0-M2 Jan 24, 2024
@mdedetrich
Copy link
Contributor

I think it makes sense to solve this when we get multi-jar-release working so we can target jdk 9+ in a principled fashion. For this reason I think removing it from the 1.1.0-M2 milestone makes sense as I seriously doubt that we will get multi-jar-release working by then, @He-Pin @pjfanning do you agree?

@He-Pin
Copy link
Member

He-Pin commented Mar 20, 2024

IIRC, the multi-jar-release has bad IDE support, and @Glavo once told me about that too, for me the current implementation seems good, so does netty work this way, another library like reactor-core is using the multi-jar-release mechanism.

@pjfanning
Copy link
Contributor Author

I don't believe in dropping useful features because one implementation that can't be done is preferred over one that is ready to go.

@mdedetrich
Copy link
Contributor

mdedetrich commented Mar 20, 2024

I am not saying that the feature should be dropped in general, just that the current implementation has a lot of unanswered questions and considering that the version with multi-jar-release would be trivial in comparison that might be preferrable.

Also since we are dealing with JDK9 specifically, we have machinery in place in Pekko to compile classes just for JDK 9

IIRC, the multi-jar-release has bad IDE support

IDE support is low priority compared to everything else and the version with reflection is much harder to understand/read code wise.

@He-Pin
Copy link
Member

He-Pin commented Mar 20, 2024

@mdedetrich Netty's PlatformDependent0 is doing the same, I don't think multi-jar-release should be a blocker for this, What about android? multi-jar-release can not ensure the code will work on Android too.

@mdedetrich
Copy link
Contributor

multi-jar-release can not ensure the code will work on Android too.

Android supports JDK 1.8+ which means it will ignore java 9 compiled classes in the multi-release-jar (its the java-9 compiled classes which will be a non standard location).

In other words it will work fine

I don't think multi-jar-release should be a blocker for this

I am not saying that its a blocker, just that multi-jar-release may already be done before the feature is complete

@He-Pin
Copy link
Member

He-Pin commented Mar 20, 2024

Some methods are missing on Android, AFAIK, we will see.

@He-Pin
Copy link
Member

He-Pin commented Mar 20, 2024

@mdedetrich It will be contagious when user packaging shadow jars, @Glavo just told me。

@pjfanning
Copy link
Contributor Author

@mdedetrich It will be contagious when user packaging shadow jars, @Glavo just told me。

I don't think this is a good reason not to use MR jars. Good shading tools support MR jars.

@pjfanning
Copy link
Contributor Author

We already depend on jackson-core which is an MR jar.

@mdedetrich
Copy link
Contributor

@mdedetrich It will be contagious when user packaging shadow jars, @Glavo just told me。

I don't think this is a good reason not to use MR jars. Good shading tools support MR jars.

Agreed, I don't know what the aversion to multi release jar's comes from but its well support and a lot of critical Java OS libraries use it.

The issues currently with multi release jar's are more sbt specific and this is largely a historical artifact (which is that any new functionality in later JDK's typically had their own equivalent idiomatic Scala specific solution so there wasn't much incentive to support multi-release-jars in sbt as there was very little demand for it).

@pjfanning
Copy link
Contributor Author

@He-Pin I'm open to pressing to get this into 1.1. My view is that user need outweighs implementation niceties. Do you need this change?

@He-Pin
Copy link
Member

He-Pin commented Mar 21, 2024

@pjfanning Yes, I'm using Pekko in several of our mission-critical systems, so I would like to keep my systems safe. If we like, we can re-implement it later with multi-jar-release.

@pjfanning pjfanning modified the milestones: 1.1.0-M2, 1.1.0-M1 May 6, 2024
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

No branches or pull requests

3 participants