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

feat: Enhance virtual thread support #1724

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Jan 17, 2025

Motivation:
Enhances the current virtual thread support which can switch the virtual threads' scheduler.

Modification:
Use the method handle to change the scheduler of virtual threads.

  1. when virtualization is on, the forkJoinPool will create a CarrierThread
  2. and then all actors run on a virtual threads.
  3. when virtualization is off, the actors run on the old PekkoForkJoinWorkerThread.

Result:
Virtualization with virtual threads supported.

We have a user case at $Work, where we are pulling data from a blocking API, With this, I think we can then decrease the Threads number.

based on #1734

@He-Pin He-Pin added this to the 1.2.0 milestone Jan 17, 2025
@He-Pin He-Pin requested review from pjfanning, Roiocam, raboof and jrudolph and removed request for pjfanning January 17, 2025 18:56
@He-Pin He-Pin marked this pull request as ready for review January 17, 2025 18:56
@He-Pin He-Pin requested a review from nvollmar January 17, 2025 18:57
# When set to `on` but underlying runtime does not support virtual threads, an Exception will throw.
# Virtualize this dispatcher as a virtual-thread-executor
# Valid values are: `on`, `off`
virtualize = off
Copy link
Member Author

Choose a reason for hiding this comment

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

behind an option

@He-Pin He-Pin force-pushed the loom branch 2 times, most recently from 262b627 to fc2f4d7 Compare January 17, 2025 19:01
project/JdkOptions.scala Outdated Show resolved Hide resolved
@He-Pin
Copy link
Member Author

He-Pin commented Jan 17, 2025

case ThreadPoolConfig is nowhere.

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

I think this can be written using MethodHandles. #1724 (comment)

@He-Pin

This comment was marked as outdated.

@He-Pin He-Pin marked this pull request as draft January 17, 2025 23:24
@He-Pin

This comment was marked as outdated.

@He-Pin He-Pin requested a review from pjfanning January 18, 2025 01:31
@He-Pin He-Pin marked this pull request as ready for review January 18, 2025 01:33
@He-Pin
Copy link
Member Author

He-Pin commented Jan 18, 2025

@pjfanning I took another look at vert.x, which is using reflection too, let's move fast and ship it as this, and then in 2.0.0, we can drop Java 8 completely, I'm doing this in my free time, I don't like over-engineering.

Especially with the current problem of #1729, so It can only be tested in a separate cycle.

I will leave it as it is now, I vote for shipping it as it is in 1.2.0 and then we move to Java 11 in 2.0.x, btw, there are many reflections in the current codebase too.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 18, 2025

@pjfanning If you are worried about the --add-opens, I can add a JPMS util for it, and then the support is out of the box.

@pjfanning
Copy link
Contributor

  • can we add a config to only support this if a config is set - and the config defaults to false?
  • this means that the default Java 17+ user won't need to add extra add-opens but the ones who want virtual threads can enable the config but also will need to add the add-opens?

@He-Pin
Copy link
Member Author

He-Pin commented Jan 18, 2025

We are doing that at $Work (a trust env), but it's not a good choice for open-sourcing work, where people will have their modules opened unexpectedly.

Maybe a configuration with JPMSUtils seems fine, but that's another story that lives in another PR.

@He-Pin He-Pin marked this pull request as draft January 19, 2025 04:43
@He-Pin He-Pin force-pushed the loom branch 2 times, most recently from 621f209 to ebb3bbd Compare January 19, 2025 05:06
@He-Pin He-Pin changed the title Enhance virtual thread support feat: Enhance virtual thread support Jan 19, 2025
@He-Pin
Copy link
Member Author

He-Pin commented Jan 19, 2025

can we add a config to only support this if a config is set - and the config defaults to false?

Can't, will need a add-opens at least for MethodHandles' LookUp

this means that the default Java 17+ user won't need to add extra add-opens but the ones who want virtual threads can enable the config but also will need to add the add-opens?

No, if and only if a user turns on the virtualize = on, which is an advanced feature, I can update the document after
#1734

People should use the current virtual-thread-executor support, as I said , this PR is for advance feature.

@He-Pin He-Pin marked this pull request as ready for review January 19, 2025 22:11
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.

2 participants