On Thu, 31 Oct 2024 03:52:31 GMT, Dean Long <dl...@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
