-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Experiment with fully virtual VirtualThreadPool #11501
Merged
Merged
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
3a68e56
Experiment with fully virtual VirtualThreadPool
gregw c630419
Merge branch 'jetty-12.0.x' into experiment/jetty-12.0.x/fullyVirtual
gregw 8d66b30
Update from review
gregw f722a1e
Keep virtual main thread running to prevent JVM exit
gregw e1c9e24
Merge remote-tracking branch 'origin/jetty-12.0.x' into experiment/je…
gregw 66fa0bc
revert module names
gregw d5d58cd
Merge branch 'jetty-12.0.x' into experiment/jetty-12.0.x/fullyVirtual
gregw 77e185a
Updates from review
gregw 96cb6e8
Improved test of dump to always stop spinners
gregw 86ba118
Merge remote-tracking branch 'origin/jetty-12.0.x' into experiment/je…
gregw 061be04
Updates from review
gregw 34e1ea2
Updates from review
gregw 1a225ec
updates from review
gregw 0a4e6a5
updates from review
gregw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
15 changes: 15 additions & 0 deletions
15
jetty-core/jetty-server/src/main/config/etc/jetty-threadpool-all-virtual.xml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<?xml version="1.0"?> | ||
<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "https://www.eclipse.org/jetty/configure_10_0.dtd"> | ||
|
||
<Configure> | ||
<New id="threadPool" class="org.eclipse.jetty.util.thread.VirtualThreadPool"> | ||
<Set name="name" property="jetty.threadPool.namePrefix" /> | ||
</New> | ||
|
||
<Call class="org.slf4j.LoggerFactory" name="getLogger"> | ||
<Arg>org.eclipse.jetty</Arg> | ||
<Call name="info"> | ||
<Arg>Virtual threads enabled. Using virtual threads for all Jetty tasks.</Arg> | ||
</Call> | ||
</Call> | ||
</Configure> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
19 changes: 19 additions & 0 deletions
19
jetty-core/jetty-server/src/main/config/modules/threadpool-all-virtual.mod
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
[description] | ||
Enables and configures the Server ThreadPool with support for virtual threads to be used for all threads. | ||
There is some risk of CPU pinning with this configuration. Only supported in Java 21 or later. | ||
|
||
[depends] | ||
logging | ||
|
||
[provides] | ||
threadpool | ||
|
||
[xml] | ||
etc/jetty-threadpool-all-virtual.xml | ||
|
||
[ini-template] | ||
# tag::documentation[] | ||
## Platform threads name prefix. | ||
#jetty.threadPool.namePrefix=vtp<hashCode> | ||
|
||
# end::documentation[] | ||
gregw marked this conversation as resolved.
Show resolved
Hide resolved
|
3 changes: 2 additions & 1 deletion
3
jetty-core/jetty-server/src/main/config/modules/threadpool-virtual.mod
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
102 changes: 102 additions & 0 deletions
102
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/TrackingExecutor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
// | ||
// ======================================================================== | ||
// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. | ||
// | ||
// This program and the accompanying materials are made available under the | ||
// terms of the Eclipse Public License v. 2.0 which is available at | ||
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 | ||
// which is available at https://www.apache.org/licenses/LICENSE-2.0. | ||
// | ||
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 | ||
// ======================================================================== | ||
// | ||
|
||
package org.eclipse.jetty.util.thread; | ||
|
||
import java.io.IOException; | ||
import java.util.Set; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
import java.util.concurrent.Executor; | ||
|
||
import org.eclipse.jetty.util.annotation.ManagedAttribute; | ||
import org.eclipse.jetty.util.annotation.ManagedObject; | ||
import org.eclipse.jetty.util.component.Dumpable; | ||
|
||
@ManagedObject("Tracking Executor wrapper") | ||
public class TrackingExecutor implements Executor, Dumpable | ||
{ | ||
private final Executor _threadFactoryExecutor; | ||
private final Set<Thread> _threads = ConcurrentHashMap.newKeySet(); | ||
private boolean _detailed; | ||
|
||
public TrackingExecutor(Executor executor, boolean detailed) | ||
{ | ||
_threadFactoryExecutor = executor; | ||
_detailed = detailed; | ||
} | ||
|
||
@Override | ||
public void execute(Runnable task) | ||
{ | ||
_threadFactoryExecutor.execute(() -> | ||
{ | ||
Thread thread = Thread.currentThread(); | ||
try | ||
{ | ||
_threads.add(thread); | ||
task.run(); | ||
} | ||
finally | ||
{ | ||
_threads.remove(thread); | ||
} | ||
}); | ||
} | ||
|
||
@Override | ||
public void dump(Appendable out, String indent) throws IOException | ||
{ | ||
Object[] threads = _threads.stream().map(DumpableThread::new).toArray(); | ||
Dumpable.dumpObjects(out, indent, _threadFactoryExecutor.toString() + " size=" + threads.length, threads); | ||
} | ||
|
||
public void setDetailedDump(boolean detailedDump) | ||
{ | ||
_detailed = detailedDump; | ||
} | ||
|
||
@ManagedAttribute("reports additional details in the dump") | ||
public boolean isDetailedDump() | ||
{ | ||
return _detailed; | ||
} | ||
|
||
public int size() | ||
{ | ||
return _threads.size(); | ||
} | ||
|
||
private class DumpableThread implements Dumpable | ||
{ | ||
private final Thread _thread; | ||
|
||
private DumpableThread(Thread thread) | ||
{ | ||
_thread = thread; | ||
} | ||
|
||
@Override | ||
public void dump(Appendable out, String indent) throws IOException | ||
{ | ||
if (_detailed) | ||
{ | ||
Object[] stack = _thread.getStackTrace(); | ||
Dumpable.dumpObjects(out, indent, _thread.toString(), stack); | ||
} | ||
else | ||
{ | ||
Dumpable.dumpObject(out, _thread); | ||
} | ||
} | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'd remove "Jetty" here; it is redundant and feels like "only the tasks of the Jetty implementation, so not those of the application".
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.
@sbordet I don't agree. We frequently have issues with users that use a mix of scheduling mechanisms. They might have an asynchronous Jetty Handler that uses some 3rd party async library that ends up using platform threads for its callback, then end up calling Jetty callbacks. It is important to be clear that we are not making all threads in the JVM virtual, just the ones that we dispatch.
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.
@gregw this is the
VirtualThreadPool
and "all tasks" refers to the ones submitted to this pool.There is no need to say "Jetty", it's just the task submitted to this pool.
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.
A server uses a platform thread pool, but have a proxy web application using
HttpClient
that usesVirtualThreadPool
and now we have "all Jetty tasks" but which ones, the client's or the server's?Perhaps let's add "Virtual thread available. Using virtual threads for all tasks submitted to VirtualThreadPool@1234."
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 still don't like that, as it is unclear what tasks are submitted to the VTP. It is kind of saying the obvious that the VTP will use virtual threads. We need to make the distinction between using threads for tasks calling the application vs tasks for all jetty tasks. I think the current text does that OK.
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.
@gregw I don't like it because there is no definition of what a "Jetty task" is, and I don't think we want to define that as it won't be useful to users.
We just need to make a difference between: everything is run by a virtual thread, or only web application code, so specifying "Jetty" is redundant because all tasks will be run by virtual threads, not only the "Jetty" tasks.
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.
OK, but you will get a great big "I told you so" the first time we get somebody confused that they have enabled this module, but that there are platform threads calling jetty callbacks and doing jetty things.