-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
Virtual Thread support #125
Conversation
} | ||
|
||
private[cask] object VirtualThreadBlockingHandler { | ||
private lazy val EXECUTOR = new NewThreadPerTaskExecutor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this Executor
live in the Main
object, rather than global. That would let it be more easily configured separately for unit tests or other multi-server scenarios
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, I was thinking to make it simpler, but with vt, this should be some kind of concurrency limiter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question about this:
When we expose the executor
with handlerExecutor
and then user can just override it with a Executors.newThreadPerTaskExecutor
on Java 21, then the MethodHandle
thing can be avoid.
I was expecting to support virtual thread out of box with cask.virtualThread.enabled
if and only if this systeam property set to true and running with Java 21.
So I think the solution should be:
- add
handlerExecutor
just as you suggested. - add a
cask.virtualThread.enabled
system property and run with Java 21 when user set that to true and running with Java 21, otherwise, without2
, all theMethodHandle
things seems unneeded.
I was busy on otherthings, sorry for the delay, what's your input on 2
?
Update: It's possible to wrap the virtual thread on top of an executor, with method handle too, I think that would b e nice way to go, will update this pr with that way.
Left some comments on the code. Thanks for looking into this! Some high-level things to do:
|
Great, thanks for the quic and detailed review . I will update this after work, we do see 10~15pt performance improvement and 5pt RAM reduce at internal stress testing, that numbers vary due to different workloads, but in most case,VT is a good choice ,and Nima is using virtual thread by default now. |
Update: Even the backing scheduler of Virtual thread is not the default VirtualThread[#91,cask-handler-executor-virtual-thread-0]/runnable@pool-1-thread-1
VirtualThread[#93,cask-handler-executor-virtual-thread-1]/runnable@pool-1-thread-2 With this setup, I think I can continue other part of this PR. object Compress extends cask.MainRoutes {
protected override val handlerExecutor: Executor = Executors.newFixedThreadPool(Runtime.getRuntime.availableProcessors())
@cask.decorators.compress
@cask.get("/")
def hello(): String = {
println(Thread.currentThread())
"Hello World! Hello World! Hello World!"
}
initialize()
} |
else if (System.getProperty("cask.virtualThread.enabled", "true").toBoolean) { | ||
Util.createVirtualThreadExecutor(executor).getOrElse(executor) | ||
} else executor | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lihaoyi Wdyt about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update the PR later, and still need update the CI to Java 21 to run the benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should run CI on both 11 and 21, with the virtual threads test suites only enabled on 21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the plan.
@He-Pin let me know when this is ready for review, looking forward to using virtual threads in cask! |
@He-Pin merged it |
Bump on this @He-Pin :) |
@lihaoyi Sorry for being late, We encountered a virtual thread deadlocking at work, related to the classloader. When the virtual thread and the other platform thread(not the carrier thread) both try to load the same classes or a virtual thread is been notified but there is no carrier thread(been pinned) to run the virtual thread, then deadlock. So we just changed the classloader, both in the JVM cpp and the lib. But, cask is a library, So the best we can do is add some documents about this case, and a simple way is limiting the Max concurrent Virtual threads number. I'm sorry for not updating this soon, I was a little busy at work and we had this issue and fixed it with JVM modification. So what do you think about :
|
@He-Pin I think documentation and best practices are fine. We do not have the power to change the JVM, so best we can do is tell people if they want to use JVM virtual threads with Cask what the best way of doing so is |
Bump on this. @He-Pin if you're not able to pick this up, I'll put it up again in the next set of bounties |
Sorry for the delay, I was working with our internal JVM team to workaround the synchronization in the Classloader, and another issue I currently encounter is the com-lihaoyi/mill#3168 on Windows. I will continue this this week, did spend sometime to learn the toolchian. |
Because |
Got it, if you're still on it I'll leave it to you then! Feel free to bump the required JVM version of necessary. We dont need to support Java 8 forever, e.g. requests-scala already moved to Java 11. Java is already on 21 or 22, so even jumping to Java 17 would be reasonable |
Closing due to inactivity |
Quick Summary:
230.51 / S (Worker Thread)
VS
1545.18 / S (Virtual Threads)
Motivation:
Fix #90
I did some analyse on cask, and found it's using undertow for now. and the undertow itself is using
Object lock
as a lock in the http://www.jboss.org/xnio codebase, which we can do some contribution there to remove the monitor and migrating toReentrantLock
. Which should be better than waiting next Java LTS (maybe Java 25).So, for now, I think we can just add the Virtual Thread support in cask, and then do some contribution upstream or migrating to another network layer maybe Netty?
The current changes is quite straightfoward.
Modifcation:
Main.scala
, others can follow after this PR.VirtualThreadSupport
which only works on the Runtime where Virtual Thread is available, otherwise, it returns null..NewThreadPerTaskExecutor
which will start a new Virtual Thread for every Command/Runnable, the Undertow needs this.VirtualThreadBlockingHandler
which works likeBlockingHandler
but delegates toNewThreadPerTaskExecutor
in5
, instead of the connections' worker executor.cask.virtualThread.enabled
system property with expected valuetrue|false
, and only using the Virtual Thread to runRoutes
if and only if both the Runtime supported andcask.virtualThread.enabled
is set totrue
. I'm not exposing anexecutor
or etc thing, because that's would not be the philosophy of cask, but hiding the runtime behavior behind ofcask.virtualThread.enabled
, as I was not knowing cask is running with undertow.Thread.sleep(1000)
to simulates.Results:
For the first run, I think it works just as expected, I did some assertion about the thread type
Thread.isVirtual
too during the testing.Quick Summary: 230.51 / S (Worker Thread) VS 1545.18 / S (Virtual Threads)
My Laptop : i9-14900hx + 64G RAM
Result with Connection Worker Thread Java 21:
VS
Result with Virtual Threads on Java 21
Future Actions:
NewThreadPerTaskExecutor
and testing theRoutes
is running inside a VT when all setup.