Enable VirtualThread natives by default #15704

EricYangIBM · 2022-08-11T18:13:53Z

The VirtualThread natives are needed to keep a list of every live virtual
thread for JVM TI. Since JVM TI agents can be attached at any time, enable
these natives by default.

Issue: #15183
Signed-off-by: Eric Yang eric.yang@ibm.com

EricYangIBM · 2022-08-11T18:15:24Z

I tested that the virtual thread natives are being run

jcl/src/java.base/share/classes/java/lang/System.java

runtime/oti/vmconstantpool.xml

jcl/src/java.base/share/classes/java/lang/System.java

babsingh · 2022-08-11T20:09:40Z

jenkins test sanity xlinux jdk11,jdk19

babsingh

LGTM. Will merge after the builds pass.

babsingh · 2022-08-12T14:12:54Z

https://openj9-jenkins.osuosl.org/job/Test_openjdk19_j9_sanity.functional_x86-64_linux_Personal/6/tapResults/

There is a segfault. @EricYangIBM Can you check if it is related to this PR?

cmdLineTest_J9test_common_0_FAILED

Testing: -Xlockword minimizeFootprint mode
...
Output from test:
 [ERR] Exception in thread "main" java/lang/ExceptionInInitializerError
 [ERR] 	at java/util/concurrent/atomic/Striped64.<clinit> (java.base@19-internal/Striped64.java:403)
 [ERR] 	at jdk/internal/vm/ThreadContainers$RootContainer$CountingRootContainer.<clinit> (java.base@19-internal/ThreadContainers.java:256)
 [ERR] 	at jdk/internal/vm/ThreadContainers$RootContainer.<clinit> (java.base@19-internal/ThreadContainers.java:197)
 [ERR] 	at jdk/internal/vm/ThreadContainers.root (java.base@19-internal/ThreadContainers.java:86)
 [ERR] 	at jdk/internal/vm/SharedThreadContainer.create (java.base@19-internal/SharedThreadContainer.java:100)
 [ERR] 	at java/util/concurrent/ForkJoinPool.<init> (java.base@19-internal/ForkJoinPool.java:2777)
 [ERR] 	at java/util/concurrent/ForkJoinPool.<clinit> (java.base@19-internal/ForkJoinPool.java:3778)
 [ERR] 	at java/lang/VirtualThread.lambda$createDefaultScheduler$5 (java.base@19-internal/VirtualThread.java:1045)
 [ERR] Unhandled exception
 [ERR] Type=Segmentation error vmState=0x00000000
 [ERR] J9Generic_Signal_Number=00000018 Signal_Number=0000000b Error_Value=00000000 Signal_Code=00000001
 [ERR] Handler1=00007F955A19EB80 Handler2=00007F955B7236F0 InaccessibleAddress=0000000000000008
 ...
 [ERR] Module=/home/jenkins/workspace/Test_openjdk19_j9_sanity.functional_x86-64_linux_Personal_testList_0/openjdkbinary/j2sdk-image/lib/default/libj9vm29.so
 [ERR] Module_base_address=00007F955A161000
 [ERR] Target=2_90_20220811_11 (Linux 4.4.0-170-generic)
 [ERR] CPU=amd64 (4 logical CPUs) (0x5e2f07000 RAM)
 [ERR] ----------- Stack Backtrace -----------
 [ERR] hashClassTableAt+0x6 (0x00007F955A1C1C26 [libj9vm29.so+0x60c26])
 [ERR] peekClassHashTable+0x5a (0x00007F955A1858BA [libj9vm29.so+0x248ba])
 [ERR] iterateStackTrace+0xe1c (0x00007F955A192CAC [libj9vm29.so+0x31cac])
 [ERR] internalExceptionDescribe+0x177 (0x00007F955A192E47 [libj9vm29.so+0x31e47])
 [ERR] J9VMDllMain+0x247 (0x00007F9553D0DB07 [libjclse29.so+0x4db07])
 [ERR] runJ9VMDllMain+0xc2 (0x00007F955A1B2C92 [libj9vm29.so+0x51c92])
 [ERR] pool_do+0x75 (0x00007F955A2AE8B5 [libj9vm29.so+0x14d8b5])
 [ERR] runInitializationStage+0x8e (0x00007F955A1B5B3E [libj9vm29.so+0x54b3e])
 [ERR] protectedInitializeJavaVM+0x22fd (0x00007F955A1BEA6D [libj9vm29.so+0x5da6d])
 [ERR] omrsig_protect+0x2b1 (0x00007F955B724421 [libj9prt29.so+0x2a421])
 [ERR] initializeJavaVM+0x42e (0x00007F955A1B731E [libj9vm29.so+0x5631e])
 [ERR] J9_CreateJavaVM+0x75 (0x00007F955A1AA265 [libj9vm29.so+0x49265])
 [ERR] JNI_CreateJavaVM_impl+0x91a (0x00007F955B7A305A [libjvm.so+0x1905a])
 [ERR] JNI_CreateJavaVM+0x6d2 (0x00007F955B862662 [libjvm.so+0xd662])
 [ERR] JavaMain+0x8f (0x00007F955B872A0F [libjli.so+0x4a0f])
 [ERR] ThreadJavaMain+0x9 (0x00007F955B876D09 [libjli.so+0x8d09])
 [ERR] start_thread+0xca (0x00007F955B23F6BA [libpthread.so.0+0x76ba])
 [ERR] clone+0x6d (0x00007F955AD7151D [libc.so.6+0x10751d])

EricYangIBM · 2022-08-12T14:29:41Z

The exception is from https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/3b112d4e1c05e85584248cda15d4c3c3c4c1d6be/src/java.base/share/classes/java/util/concurrent/atomic/Striped64.java#L388-L389

CELLSBUSY = l1.findVarHandle(Striped64.class,
                    "cellsBusy", int.class);

java.lang.NoSuchFieldException: no such field: java.util.concurrent.atomic.Striped64.cellsBusy/int/getField
Maybe it's related to -Xlockword minimizeFootprint?

babsingh · 2022-08-12T15:57:49Z

Maybe it's related to -Xlockword minimizeFootprint?

The problem is the segfault from native code. If the failure is introduced due to the changes from this PR, then we will have to rework these changes.

Launched a grinder: https://openj9-jenkins.osuosl.org/job/Grinder/1247/. The failure occurs consistently.

EricYangIBM · 2022-08-12T16:02:41Z

Isn't the crash from [ERR] internalExceptionDescribe+0x177 (0x00007F955A192E47 [libj9vm29.so+0x31e47])? Caused by the exception

babsingh · 2022-08-12T16:13:18Z

Generally, the VM should not crash for an exception. The new code in this PR is causing the VirtualThread class to be loaded. This leads to the evaluation of the static fields: DEFAULT_SCHEDULER = createDefaultScheduler(). I believe that the VM is not fully prepared to handle this code from System.completeInitialization. This results in the segfault. @tajila Is there another location to set the VirtualThread.notifyJvmtiEvents field?

EricYangIBM · 2022-08-12T16:53:27Z

It is crashing because in J9VMDllMain() it calls completeInitialization() which doesn't internalInitializeJavaLangClassLoader() if sendCompleteInitialization() fails, then applicationClassLoader is left NULL and later causes a crash. Maybe one of the functions called by internalExceptionDescribe should check for this

babsingh · 2022-08-12T18:46:42Z

Maybe one of the functions called by internalExceptionDescribe should check for this

Fixing internalExceptionDescribe should not be the focus. It will be easier and quicker to relocate this code. For the time being, an issue should be opened if a real problem is identified with internalExceptionDescribe.

Since the VirtualThread class init can fail at this location in some cases (ExceptionInInitializerError), then it is not guaranteed that notifyJvmtiEvents will be always set successfully. To meet our goal, we will need to move this code to a location where the field is always set successfully before any virtual threads are created.

babsingh · 2022-08-12T19:08:03Z

Ref: ibmruntimes/openj9-openjdk-jdk19#18 (comment)

https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/openj9/src/java.base/share/classes/java/lang/VirtualThread.java#L1008-L1012

openj9/runtime/jcl/common/thread.cpp

Lines 619 to 623 in 92da2c6

    
           /* private static native void registerNatives(); */ 
        
           void JNICALL 
        
           Java_java_lang_VirtualThread_registerNatives(JNIEnv *env, jclass clazz) 
        
           { 
        
           }

Here, clazz is already an input. Does setting the field from VirtualThread.registerNatives work? It will only be initialized if the VirtualThread class is ever used.

        jfieldID vthreadFieldID = (*env)->GetStaticFieldID(env, clazz, "notifyJvmtiEvents", "Z");
        if (NULL != vthreadFieldID) {
            (*env)->SetStaticBooleanField(env, clazz, vthreadField, JNI_TRUE);
        }

I would even add an assert if the field is not found. If the Java code ever changes, the assertion will inform us to take action and update our implementation.

EricYangIBM · 2022-08-12T19:19:35Z

setting the field from VirtualThread.registerNatives

Wouldn't that be too late since a jvmti agent can be attached by then?

babsingh · 2022-08-12T19:29:20Z

Wouldn't that be too late since a jvmti agent can be attached by then?

No. The field is only used within the VirtualThread class. It should be set before any VirtualThread instances are created. The static block for registerNatives will always be invoked before any VirtualThread instances are created. Even if a JVMTI agent is attached, setting the field is not needed until the VirtualThread class is used.

The VirtualThread natives are needed to keep a list of every live virtual thread for JVM TI. Since JVM TI agents can be attached at any time, enable these natives by default. Issue: eclipse-openj9#15183 Signed-off-by: Eric Yang <eric.yang@ibm.com>

babsingh · 2022-08-15T14:41:13Z

jenkins test sanity win jdk19

EricYangIBM · 2022-08-15T19:47:06Z

Previous failures in xlinux passing: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/13841/

keithc-ca · 2022-08-15T21:25:37Z

It seems problematic to me that we have two implementations of VirtualThread.registerNatives(), one in libjclse29.so and the other in libjava.so. I think we should instead be (properly) implementing the functions JVM_VirtualThread* and deferring to the openjdk natives. Of course that means this change needs to find a new home.

babsingh · 2022-08-16T15:35:03Z

re #15704 (comment):

@keithc-ca I will verify if we can reuse the extension repo implementation of VirtualThread.registerNatives(). If it does not work, can we remove it?

@EricYangIBM Which version of VirtualThread.registerNatives() gets invoked: ours or extension repo?

keithc-ca · 2022-08-16T15:40:18Z

It seems our version of VirtualThread.registerNatives() gets used - at least in the simple test case I put together, but I don't know that is guaranteed.

#15717 fixes the signatures of the JVM_VirtualThread* functions. If we remove our version of VirtualThread.registerNatives(), the one from openjdk will be used; it uses the JVM_VirtualThread* functions, so we would need to move the implementation of the mount/unmount natives there.

We will need to set notifyJvmtiEvents somewhere else.

EricYangIBM · 2022-08-16T16:52:56Z

Note: Thread also has two registerNatives implementations

keithc-ca · 2022-08-16T16:58:17Z

Note: Thread also has two registerNatives implementations

No, in the extensions repositories, closed/src/java.base/share/native/libjava/Thread.c hides src/java.base/share/native/libjava/Thread.c.

EricYangIBM · 2022-08-18T18:46:11Z

Could we hide their version of registerNatives in the same way? Just as a short term fix for the duplication.

keithc-ca · 2022-08-18T18:49:36Z

My suggestion is that we remove our version in favour of theirs and move the mount/unmount implementations into the related JVM_VirtualThread* functions (that now have the proper signatures).

EricYangIBM · 2022-08-18T19:19:15Z

The problem is that we currently do not have a better place to set notifyJvmtiEvents. It must be set after vm initialization but before a virtual thread can be run. Setting it in System.completeInitialization is too early as seen in the above comments.

babsingh · 2022-08-18T20:14:45Z

Few other problems: Moving code from VirtualThread_notifyJVMTI* into JVM_VirtualThread* won't be straightforward since omrthread_monitor_* functions are not linked to javanextvmi.c (linkage error seen). Also, more input parameter sanitation may be needed in the JVM_VirtualThread* functions assuming these functions are standalone and can be used independently. Currently, the JVM_VirtualThread* functions are only invoked from the VirtualThread class in the RI. So, I do not see any new functional value in adopting their VirtualThread.registerNatives implementation and implementing the JVM_VirtualThread* functions. We have adopted OJDK's j.l.Thread but we use our implementation of registerNatives to better control interoperability with OpenJ9. We should do the same with j.l.VirtualThread, and exclude/hide their implementation.

keithc-ca · 2022-08-19T15:44:03Z

I don't understand why you see (or expect) a linkage error; of course, we'll have to defer to openjdk's registerNatives
if openjdk makes any other uses of the JVM_VirtualThread* functions we will have to implement them (instead of asserting they're unused)
I don't see any real need for parameter checking than what we already have; the JVM_VirtualThread* functions have the same signatures as the VirtualThread_notifyJVMTI* functions (and they must because openjdk declares they're the implementation of the native methods of VirtualThread)
using registerNatives for something other than registering native methods just seems wrong

In my view, our current approach has more potential problems than what I'm proposing.

keithc-ca · 2022-08-19T16:12:08Z

The problem is that we currently do not have a better place to set notifyJvmtiEvents. It must be set after vm initialization but before a virtual thread can be run. Setting it in System.completeInitialization is too early as seen in the above comments.

Perhaps the best answer for now is to modify VirtualThread.java to set notifyJvmtiEvents after the call to registerNatives().

babsingh · 2022-08-19T16:20:54Z

I am not against these changes. I have prototyped them earlier this week. I have opened a PR: #15755.

I don't understand why you see (or expect) a linkage error; of course, we'll have to defer to openjdk's registerNatives

The linkage error is seen after the code is moved from VirtualThread_notifyJVMTI* into JVM_VirtualThread*. JVM_VirtualThread*. The JVM_VirtualThread* functions exist in javanextvmi.c, and this file does not have visibility to the omrthread_monitor_* functions. It should be fixable; it will require more code + time.

if openjdk makes any other uses of the JVM_VirtualThread* functions we will have to implement them (instead of asserting they're unused)

A potential future issue, which does not immediately impact us. A number of PRs are open where VirtualThread_notifyJVMTI* functions are being modified. I would delay this change until the implementation of VirtualThread_notifyJVMTI* functions are finalized in order to save time from rebasing those PRs.

using registerNatives for something other than registering native methods just seems wrong

You are correct. But, we have used this approach as a workaround in the past for a similar case.

openj9/runtime/jcl/common/sun_misc_Unsafe.cpp

Lines 192 to 207 in 984612f

    
           Java_sun_misc_Unsafe_registerNatives(JNIEnv *env, jclass clazz) 
        
           { 
        
           	jfieldID fid; 
        
           	Trc_JCL_sun_misc_Unsafe_registerNatives_Entry(env); 
        
           	/* If Unsafe has a static int field called INVALID_FIELD_OFFSET, set it to -1 */ 
        
           	fid = env->GetStaticFieldID(clazz, "INVALID_FIELD_OFFSET", "I"); 
        
           	if (fid == NULL) { 
        
           		env->ExceptionClear(); 
        
           	} else { 
        
           		env->SetStaticIntField(clazz, fid, -1); 
        
           	} 
        
           	Trc_JCL_sun_misc_Unsafe_registerNatives_Exit(env);

Perhaps the best answer for now is to modify VirtualThread.java to set notifyJvmtiEvents after the call to registerNatives().

This was the initial approach: ibmruntimes/openj9-openjdk-jdk19#18.

See comments at the end of eclipse-openj9#15704 for more details. Converted javanextvmi.c to a CPP file for including VMHelpers.hpp. Depends on ibmruntimes/openj9-openjdk-jdk19#23. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

EricYangIBM mentioned this pull request Aug 11, 2022

Enable VirtualThread natives by default ibmruntimes/openj9-openjdk-jdk19#18

Closed

EricYangIBM requested a review from babsingh August 11, 2022 18:15

babsingh reviewed Aug 11, 2022

View reviewed changes

jcl/src/java.base/share/classes/java/lang/System.java Outdated Show resolved Hide resolved

babsingh reviewed Aug 11, 2022

View reviewed changes

runtime/oti/vmconstantpool.xml Outdated Show resolved Hide resolved

babsingh reviewed Aug 11, 2022

View reviewed changes

jcl/src/java.base/share/classes/java/lang/System.java Outdated Show resolved Hide resolved

EricYangIBM force-pushed the notifyJvmtiEvents branch from d678473 to 55234a9 Compare August 11, 2022 19:51

babsingh reviewed Aug 11, 2022

View reviewed changes

EricYangIBM force-pushed the notifyJvmtiEvents branch from 55234a9 to 37a99fd Compare August 15, 2022 14:39

EricYangIBM force-pushed the notifyJvmtiEvents branch from 37a99fd to 1fcfe29 Compare August 15, 2022 14:39

babsingh approved these changes Aug 15, 2022

View reviewed changes

babsingh merged commit 59e8348 into eclipse-openj9:master Aug 15, 2022

EricYangIBM deleted the notifyJvmtiEvents branch August 17, 2022 19:17

babsingh mentioned this pull request Aug 19, 2022

Use JVM_VirtualThread* functions correctly with Virtual Threads #15755

Merged

This was referenced Aug 19, 2022

Block Virtual Threads for JVMTI Inspection #15690

Closed

Enable VirtualThread JVMTI natives by default ibmruntimes/openj9-openjdk-jdk19#23

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable VirtualThread natives by default #15704

Enable VirtualThread natives by default #15704

EricYangIBM commented Aug 11, 2022

EricYangIBM commented Aug 11, 2022

babsingh commented Aug 11, 2022

babsingh left a comment •

edited

Loading

babsingh commented Aug 12, 2022

EricYangIBM commented Aug 12, 2022

babsingh commented Aug 12, 2022

EricYangIBM commented Aug 12, 2022

babsingh commented Aug 12, 2022 •

edited

Loading

EricYangIBM commented Aug 12, 2022

babsingh commented Aug 12, 2022 •

edited

Loading

babsingh commented Aug 12, 2022 •

edited

Loading

EricYangIBM commented Aug 12, 2022

babsingh commented Aug 12, 2022 •

edited

Loading

babsingh commented Aug 15, 2022

EricYangIBM commented Aug 15, 2022

keithc-ca commented Aug 15, 2022

babsingh commented Aug 16, 2022

keithc-ca commented Aug 16, 2022

EricYangIBM commented Aug 16, 2022

keithc-ca commented Aug 16, 2022

EricYangIBM commented Aug 18, 2022

keithc-ca commented Aug 18, 2022

EricYangIBM commented Aug 18, 2022

babsingh commented Aug 18, 2022 •

edited

Loading

keithc-ca commented Aug 19, 2022

keithc-ca commented Aug 19, 2022

babsingh commented Aug 19, 2022

Enable VirtualThread natives by default #15704

Enable VirtualThread natives by default #15704

Conversation

EricYangIBM commented Aug 11, 2022

EricYangIBM commented Aug 11, 2022

babsingh commented Aug 11, 2022

babsingh left a comment • edited Loading

Choose a reason for hiding this comment

babsingh commented Aug 12, 2022

EricYangIBM commented Aug 12, 2022

babsingh commented Aug 12, 2022

EricYangIBM commented Aug 12, 2022

babsingh commented Aug 12, 2022 • edited Loading

EricYangIBM commented Aug 12, 2022

babsingh commented Aug 12, 2022 • edited Loading

babsingh commented Aug 12, 2022 • edited Loading

EricYangIBM commented Aug 12, 2022

babsingh commented Aug 12, 2022 • edited Loading

babsingh commented Aug 15, 2022

EricYangIBM commented Aug 15, 2022

keithc-ca commented Aug 15, 2022

babsingh commented Aug 16, 2022

keithc-ca commented Aug 16, 2022

EricYangIBM commented Aug 16, 2022

keithc-ca commented Aug 16, 2022

EricYangIBM commented Aug 18, 2022

keithc-ca commented Aug 18, 2022

EricYangIBM commented Aug 18, 2022

babsingh commented Aug 18, 2022 • edited Loading

keithc-ca commented Aug 19, 2022

keithc-ca commented Aug 19, 2022

babsingh commented Aug 19, 2022

babsingh left a comment •

edited

Loading

babsingh commented Aug 12, 2022 •

edited

Loading

babsingh commented Aug 12, 2022 •

edited

Loading

babsingh commented Aug 12, 2022 •

edited

Loading

babsingh commented Aug 12, 2022 •

edited

Loading

babsingh commented Aug 18, 2022 •

edited

Loading