-
Notifications
You must be signed in to change notification settings - Fork 821
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
Provide hints for conversion to Virtual thread executor when thread pools are used #4592
Conversation
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.
Added a few comments on where I think the hint could be improved. But, I wonder if "blindly" suggesting a conversion to newVirtualThreadPerTaskExecutor
is desirable - doesn't newVirtualThreadPerTaskExecutor
work best for tasks that wait a lot e.g. on I/O?
Also, there are probably patterns that should not be re-written, like Executors.newFixedThreadPool(1)
- that is likely to mean run on background, but single-threaded. Changing this to multiple (virtual or non-virtual) threads may lead to problems.
java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToVT.java
Outdated
Show resolved
Hide resolved
java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToVT.java
Outdated
Show resolved
Hide resolved
java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToVT.java
Outdated
Show resolved
Hide resolved
java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToVT.java
Outdated
Show resolved
Hide resolved
java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToVT.java
Outdated
Show resolved
Hide resolved
java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToVT.java
Outdated
Show resolved
Hide resolved
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.
we should discuss if this is something the IDE should recommend by default. Because switching to VTs is not something which should be done blindly.
Even though the API is the same, the concepts are not. E.g there usually is no reason to migrate purely CPU bound tasks (as correctly mentioned in the hint description).
But more importantly: fixed thread pools are not the same as a newVirtualThreadPerTaskExecutor
, you going to need a semaphore to achieve similar behavior otherwise you just added a DDOS vuln. There are also other things to pay attention to (in both your code and in libraries), e.g usage of ThreadLocals
which used to be an optimization, but are now an anti-pattern which doesn't scale anymore. Or synchronized sections which can block or native stack frames which could lock carrier threads etc.
VTs are great and I am all for using new features and having code transformations which help with migration but maybe not enabled by default :)
options to consider: disable the hint, or make it current-line only
@neilcsmith-net @matthiasblaesing whats your opinions on this?
Emphasis mine, but yes, agree, IMO should not be enabled by default, for now at least. |
Based on the comments we have now changed to give hints for the new feature of VT thats available. |
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.
great addition - helps to find VT candidates
…rtualThreadHints
@singh-akhilesh please make sure there's a milestone set when merging, thanks! |
…ools are used (apache#4592) * add hints for using virtual threads
Provide hints for converting thread pools(newFixedThreadPool, newCachedThreadPool) to virtual thread pool executor.
When ThreadFactory is passed as argument to , then only a hint suggestion for converting the thread pool to newThreadPerTaskExecutor is provided. Code is not directly rewritten in this case.