-
Notifications
You must be signed in to change notification settings - Fork 62
Use Virtual Threads when on JDK 24+ #74
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
base: master
Are you sure you want to change the base?
Conversation
|
I've had a quick look over your proposed changes (note that I'm not very familiar with kwik as a project, but I have worked quite a bit with VirtualThreads). I think the changes look good, as far as that it shouldn't break anything, but I think it falls a bit short conceptually. Currently the factory which you have changed supplies several ThreadPools in kwik. Generally speaking, you should never pool VirtualThreads, because pooling as a concept only makes sense for expensive resources. A PlatformThread (i.e. a normal OS thread) is an expensive resource, a VirtualThread however is not (the bookkeeping effort of pooling the thread may very much outweigh to cost of creating a new one). So in my opinion, if you want to introduce VirtualThreads, you should also adapt the ThreadPools which consume them (i.e. probably just make them non-caching in JDK24+). You also don't have to take my word for this, the original JEP which introduced VirtualThreads in JDK 21 already makes that argument. It also goes into quite some detail about what the mental model for this feature is, and even though some things (mostly limitations like monitor pinning) have changed since JDK 21, I think it still gives a good birds-eye view of the whole situation. I think it's definitely worth a read if your looking to implement something on top of VirtualThreads. (this is not meant as criticism, I was just pinged by your issue comment and wanted to offer my two cents on the topic :) ) |
Yeah I was going to do a follow up to modify all of those cached threadpools into thread per task executors. For the scheduled executors I figured there was no harm in using a virtual thread factory,. |
|
though I suppose there is no need to wait |
|
@ptrd what do we think about this? |
This merges two files as-is from the related PR. It then uses the new `VirtualExecutor` system where the single-thread executor was previously being used as part of these async changes. Related-to: ptrd#74 Co-authored-by: Josiah Noel <32279667+SentryMan@users.noreply.github.com> Signed-off-by: Mahied Maruf <contact@mechite.com>
Thread.ofVirtual().name(threadBaseName, 0).factory()when on jdk 24+.VirtualExecutorto reflectively create thread per task executorspart of #53