RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning

[Alan Bateman]

On Thu, 31 Oct 2024 03:52:31 GMT, Dean Long <dlong at openjdk.org> wrote:

> For some reason github thinks VirtualThreadPinnedEvent.java was renamed to libSynchronizedNative.c and libTracePinnedThreads.c was renamed to LockingMode.java. Is there a way to fix that?

I don't know which view this is but just to say that VirtualThreadPinnedEvent.java and libTracePinnedThreads.c are removed. libSynchronizedNative.c is part of a new test (as it happens, it was previously reviewed as pull/18600 but we had to hold it back as it needed a fix from the loom repo that is part of the JEP 491 implementation). You find is easier to just fetch and checkout the branch to look at the changes locally. Personally I have this easier for large change and makes it easier to see renames and/or removals.

> src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 108:
> 
>> 106:         processDeregisterQueue();
>> 107: 
>> 108:         if (Thread.currentThread().isVirtual()) {
> 
> It looks like we have two implementations, depending on if the current thread is virtual or not.  The two implementations differ in the way they signal interrupted.  Can we unify the two somehow?

When executed on a platform thread is will block in epoll_wait or kqueue so it has to handle EINTR. It doesn't block in sys call when executed in a virtual thread. So very different implementations.

> src/java.base/share/classes/sun/security/ssl/X509TrustManagerImpl.java line 57:
> 
>> 55:     static {
>> 56:         try {
>> 57:             MethodHandles.lookup().ensureInitialized(AnchorCertificates.class);
> 
> Why is this needed?  A comment would help.

That's probably a good idea.  It’s caused by pinning due to the sun.security.util.AnchorCertificates’s class initializer, some of the http client tests are running into this. Once monitors are out of the way then class initializers, both executing, and waiting for, will be a priority.

> test/hotspot/gtest/nmt/test_vmatree.cpp line 34:
> 
>> 32: 
>> 33: using Tree = VMATree;
>> 34: using TNode = Tree::TreapNode;
> 
> Why is this needed?

We had to rename the alias to avoid a conflict with the Node in compile.hpp. Just lucky not to run into this in main-line. It comes and goes, depends on changes to header files that are transitively included by the test. I think Johan had planned to change this in main line but it may have got forgotten.

> test/hotspot/jtreg/compiler/codecache/stress/OverloadCompileQueueTest.java line 42:
> 
>> 40:  *                   -XX:CompileCommand=exclude,java.lang.Thread::beforeSleep
>> 41:  *                   -XX:CompileCommand=exclude,java.lang.Thread::afterSleep
>> 42:  *                   -XX:CompileCommand=exclude,java.util.concurrent.TimeUnit::toNanos
> 
> I'm guessing these changes have something to do with JDK-8279653?

It should have been added when Thread.sleep was changed but we got lucky.

> test/hotspot/jtreg/serviceability/jvmti/events/MonitorContendedEnter/mcontenter01/libmcontenter01.cpp line 73:
> 
>> 71: /* ========================================================================== */
>> 72: 
>> 73: static int prepare(JNIEnv* jni) {
> 
> Is this a bug fix?

Testing ran into a couple of bugs in JVMTI tests. One of was tests that was stashing the JNIEnv into a static.

> test/jdk/java/lang/reflect/callerCache/ReflectionCallerCacheTest.java line 30:
> 
>> 28:  *         by reflection API
>> 29:  * @library /test/lib/
>> 30:  * @requires vm.compMode != "Xcomp"
> 
> If there is a problem with this test running with -Xcomp and virtual threads, maybe it should be handled as a separate bug fix.

JBS has several issues related to ReflectionCallerCacheTest.java and -Xcomp, going back several releases. It seems some nmethod is keeping objects alive and is preventing class unloading in this test. The refactoring of j.l.ref in JDK 19 to workaround pinning issues made it go away. There is some minimal revert in this PR to deal with the potential for preemption when polling a reference queue and it seems the changes to this Java code have brought back the issue. So it's excluded from -Xcomp again.  Maybe it would be better to add it to ProblemList-Xcomp.txt instead? That would allow it to link to one of the JSB issue on this issue.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/21565#issuecomment-2449153774
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825115214
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825127591
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825121520
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825112326
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825110254
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817692430