Experiment with fully virtual VirtualThreadPool

[gregw]

Virtual threads are used for all threading.

[gregw]

This will also need #11504

[sbordet]

If a selector thread is virtual, then it's pinned when it is waiting in select().

It is difficult to show with jstack, but using a mix of jstack and a call to the MBean HotspotDiagnostic.dumpThreads("/tmp/jvm.dump", JSON), and with system property -Djdk.trackAllThreads=true, you get, for example:

From jstack

"ForkJoinPool-1-worker-1" #31 [58094] daemon prio=5 os_prio=0 cpu=1.15ms elapsed=963.80s allocated=22128B defined_classes=5 tid=0x000074ee24d6f450  [0x000074edf482c000]
   Carrying virtual thread #30
	at jdk.internal.vm.Continuation.run(java.base@21.0.2/Continuation.java:248)
	at java.lang.VirtualThread.runContinuation(java.base@21.0.2/VirtualThread.java:221)
	at java.lang.VirtualThread$$Lambda/0x00000007c01ffa08.run(java.base@21.0.2/Unknown Source)
	at java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(java.base@21.0.2/ForkJoinTask.java:1423)
	at java.util.concurrent.ForkJoinTask.doExec(java.base@21.0.2/ForkJoinTask.java:387)
	at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(java.base@21.0.2/ForkJoinPool.java:1312)
	at java.util.concurrent.ForkJoinPool.scan(java.base@21.0.2/ForkJoinPool.java:1843)
	at java.util.concurrent.ForkJoinPool.runWorker(java.base@21.0.2/ForkJoinPool.java:1808)
	at java.util.concurrent.ForkJoinWorkerThread.run(java.base@21.0.2/ForkJoinWorkerThread.java:188)

From the /tmp/jvm.dump file:

         {
           "tid": "30",
           "name": "server0",
           "stack": [
              "java.base\/sun.nio.ch.EPoll.wait(Native Method)",
              "java.base\/sun.nio.ch.EPollSelectorImpl.doSelect(EPollSelectorImpl.java:121)",
              "java.base\/sun.nio.ch.SelectorImpl.lockAndDoSelect(SelectorImpl.java:130)",
              "java.base\/sun.nio.ch.SelectorImpl.select(SelectorImpl.java:147)",
              "org.eclipse.jetty.io@12.0.7-SNAPSHOT\/org.eclipse.jetty.io.ManagedSelector.nioSelect(ManagedSelector.java:181)",
              "org.eclipse.jetty.io@12.0.7-SNAPSHOT\/org.eclipse.jetty.io.ManagedSelector.select(ManagedSelector.java:188)",
              "org.eclipse.jetty.io@12.0.7-SNAPSHOT\/org.eclipse.jetty.io.ManagedSelector$SelectorProducer.select(ManagedSelector.java:612)",
              "org.eclipse.jetty.io@12.0.7-SNAPSHOT\/org.eclipse.jetty.io.ManagedSelector$SelectorProducer.produce(ManagedSelector.java:546)",
              "org.eclipse.jetty.util@12.0.7-SNAPSHOT\/org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produceTask(AdaptiveExecutionStrategy.java:512)",
              "org.eclipse.jetty.util@12.0.7-SNAPSHOT\/org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:258)",
              "org.eclipse.jetty.util@12.0.7-SNAPSHOT\/org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produce(AdaptiveExecutionStrategy.java:195)",
              "java.base\/java.util.concurrent.ThreadPerTaskExecutor$TaskRunner.run(ThreadPerTaskExecutor.java:314)",
              "java.base\/java.lang.VirtualThread.run(VirtualThread.java:309)"
           ]
         },

[sbordet]

ForkJoinPool, which is used as the default scheduler for virtual threads, can be constructed using a "parallelism" and a "maxPoolSize".

By default, parallelism=#cores and maxPoolSize=256 (see VirtualThread.createDefaultScheduler() in the JDK sources).

This means that if we have 8 cores and 8 selectors, we would be completely pinned, if not that FJP can "overshoot" and create other platform threads, up to 256 total.

Setting -Djdk.virtualThreadScheduler.maxPoolSize=8 would limit the overshoot to 8, but if you have 8 virtual threads pinned in select() now the system is completely pinned.

The default idle timeout for the virtual thread FJP scheduler is 30 seconds, so they stay around for a while.

The option of only using virtual threads starts to feel in the camp of "magic", as in you think you know how the system works, but not really, until you know the details above.

[lorban]

We should test this change with the perf profiler but #11513 should be fixed first.

[gregw]

@sbordet @lorban because of pinning, I doubt this is a viable mode to run it.... but it would still be interesting to include this branch benchmarks just to see what a fully virtual server would run like. This would remove the overheads of both QTP and RTE, but with the risk of parallel slowdown. It might still be a fast (if somewhat risky) mode to run in?
If we ever merge this, then we probably should modify the selector heuristics to never have more selectors than cores.

[gregw]

Considering pinning, I think the current virtual thread mode is safe because we force reserved threads to 0, thus we will never tryExecute a producer task, so the producer should never be virtual. We only use virtual threads to consume BLOCKING tasks, which should not be selectors.... but may be other code that pins the cores, but that should not be us at least.

[gregw]

@sbordet @lorban I reverted the name threadpool-hybrid to threadpool-virtual. The new mode is now called threadpool-all-virtual.

[gregw]

@sbordet @lorban thoughts on this? Is this something we want to play with, or is pinning just too much of an issue to make it worthwhile?

[sbordet]

@gregw keep it! 😄

Various comments:

1. We tried to debug a virtual thread issue using what recommended by by JEP 444 in section Observing virtual threads.
   Unfortunately, jcmd outputs a limited dump that does not report enough information (e.g. does not associate the virtual thread with its carrier, so there is no information if a virtual thread is pinned).
   The TrackingVirtualExecutor in this PR will be able to do so, provided that dumping capabilities are added.
   I have just added this:

@Override
public void dump(Appendable out, String indent) throws IOException
{
    dumpObjects(out, indent, new DumpableCollection("virtual threads", _threads.stream().map(t -> new DumpableCollection(t.toString(), List.of(t.getStackTrace()))).toList()));
}

to get state & carrier information, and stack frames even of parked virtual threads.
This would be invaluable.
However, TrackingVirtualExecutor should be a reusable class that I can use when doing:

QueuedThreadPool qtp = new QueuedThreadPool();
qtp.setVirtualThreadExecutor(new TrackingVirtualExecutor(Executors.newThreadPerTaskExecutor()));

2. I'd get rid of ThreadFactoryExecutor to use the established patter that if you need a ThreadFactory, it is passed as a parameter.
3. By default, carrier threads can overshoot up to 256, unless explicitly specified with system property jdk.virtualThreadScheduler.maxPoolSize.
   Therefore, I think there may be value to have a VirtualThreadPool (although the name is an anti-pattern 😄) as, by default, even if we pin virtual threads that do select(), the JDK overshoots up to 256 so there should be room to spare for carriers, even when setting selectors=cores, which would pin all "default" virtual threads, but the JDK would still allocate more carriers (unless you have 256 cores).
4. I'd rename the -all- with -full-.

[sbordet]

Just the virtual thread tracking capabilities are a must-have.
The rest needs cleanup IMHO, but let's continue on this PR.

[gregw]

@olamy the CI build is green. Any idea how I can make it update here?

[sbordet]

Should be more clear as in "Virtual threads enabled, using only virtual threads".

Also the file name should replace "-all-" with "-full-" or "-only-".

[gregw]

I prefer "all". "full" is confusing when pools are involved. What does it mean to have a full non pooling pool? "Only" is ok, but it is just as easy to say "the threads used are all virtual", as it is to say "the threads used are only virtual". However, "only virtual" can be taken as a pejorative (which in this case might be appropriate :), so I prefer "all"

[joakime]

I like jetty-threadpool-only-virtual.xml

[olamy]

@olamy the CI build is green. Any idea how I can make it update here?

fixed.

[gregw]

@lorban @sbordet nudge!

[lorban]

l.signal() seems enough here.

[gregw]

ya'd think! But the join test fails with just signal, I assume because multiple threads can join and wait.

[gregw]

@sbordet sorry I missed a few comments. I'm having troubling making the new intellij review mode update always. I think I have got them all now.

[sbordet]

I'd remove "Jetty" here; it is redundant and feels like "only the tasks of the Jetty implementation, so not those of the application".

[gregw]

@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.

[sbordet]

@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.

[sbordet]

A server uses a platform thread pool, but have a proxy web application using HttpClient that uses VirtualThreadPool 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."

[gregw]

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.

[sbordet]

@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.

[gregw]

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.

[sbordet]

See comment.

[joakime]

Generally I'm good with this.
Just some naming niggles, but @sbordet is on the same page as I am there.
Perhaps this should be done for Jetty 12.1.0? That would encourage people to update to 12.1.0 as well.