Re: RFR: 8329596: Add test for virtual threads invoking synchronized native methods
On Wed, 3 Apr 2024 10:52:10 GMT, Alan Bateman wrote: > This is a test-only addition to add a test for virtual threads invoking a > synchronized native method and invoking a native method that enter/exits a > monitor with JNI MonitorEnter/MonitorExit. The test has been in the loom repo > for some time, it can move to the main line in advance of changes to the > object monitor implementation. The java side of this test looks OK to me. I see that for testing concurrent/contended access we aren't doing nested monitor enters. Does it deserve to be tested? - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18600#pullrequestreview-2003899439
Re: RFR: 8329596: Add test for virtual threads invoking synchronized native methods
On Wed, 3 Apr 2024 10:52:10 GMT, Alan Bateman wrote:
> This is a test-only addition to add a test for virtual threads invoking a
> synchronized native method and invoking a native method that enter/exits a
> monitor with JNI MonitorEnter/MonitorExit. The test has been in the loom repo
> for some time, it can move to the main line in advance of changes to the
> object monitor implementation.
Nit, maybe make all re‑entrancy comments match each other:
test/jdk/java/lang/Thread/virtual/SynchronizedNative.java line 94:
> 92: VThreadRunner.run(() -> {
> 93:
> 94: // enter, reenter with a synchronized native method
Suggestion:
// enter with synchronized statement, reenter with synchronized
native method
test/jdk/java/lang/Thread/virtual/SynchronizedNative.java line 265:
> 263: VThreadRunner.run(() -> {
> 264:
> 265: // enter, reenter with JNI MonitorEnter
Suggestion:
// enter with synchronized statement, reenter with JNI MonitorEnter
-
PR Review: https://git.openjdk.org/jdk/pull/18600#pullrequestreview-1985402714
PR Review Comment: https://git.openjdk.org/jdk/pull/18600#discussion_r1555196757
PR Review Comment: https://git.openjdk.org/jdk/pull/18600#discussion_r1555196933
Re: RFR: 8329596: Add test for virtual threads invoking synchronized native methods
On Fri, 5 Apr 2024 05:17:44 GMT, Alan Bateman wrote:
>> test/jdk/java/lang/Thread/virtual/libSynchronizedNative.c line 29:
>>
>>> 27: Java_SynchronizedNative_runWithSynchronizedNative(JNIEnv *env, jobject
>>> obj, jobject task) {
>>> 28: jclass clazz = (*env)->GetObjectClass(env, obj);
>>> 29: jmethodID mid = (*env)->GetMethodID(env, clazz, "run",
>>> "(Ljava/lang/Runnable;)V");
>>
>> Does this generate a warning with `-Xcheck:jni` due to exceptions not being
>> checked for?
>
>> Does this generate a warning with `-Xcheck:jni` due to exceptions not being
>> checked for?
>
> I run with`TEST_OPTS_JAVA_OPTIONS=-Xcheck:jni` and don't see any warnings.
> Are you asking about the usage of GetObjectClass (which is not documented to
> throw) or GetMethodID (the check for the jmethodID of NULL is at L30). Or
> maybe you are saying that GetMethodID can return a jmethodID with a pending
> exception?
Sorry I was thinking `GetObjectClass` could throw, but it can't.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18600#discussion_r1552955426
Re: RFR: 8329596: Add test for virtual threads invoking synchronized native methods
On Fri, 5 Apr 2024 01:11:49 GMT, David Holmes wrote: > Does this generate a warning with `-Xcheck:jni` due to exceptions not being > checked for? I run with`TEST_OPTS_JAVA_OPTIONS=-Xcheck:jni` and don't see any warnings. Are you asking about the usage of GetObjectClass (which is not documented to throw) or GetMethodID (the check for the jmethodID of NULL is at L30). Or maybe you are saying that GetMethodID can return a jmethodID with a pending exception? - PR Review Comment: https://git.openjdk.org/jdk/pull/18600#discussion_r1552938067
Re: RFR: 8329596: Add test for virtual threads invoking synchronized native methods
On Wed, 3 Apr 2024 10:52:10 GMT, Alan Bateman wrote:
> This is a test-only addition to add a test for virtual threads invoking a
> synchronized native method and invoking a native method that enter/exits a
> monitor with JNI MonitorEnter/MonitorExit. The test has been in the loom repo
> for some time, it can move to the main line in advance of changes to the
> object monitor implementation.
test/jdk/java/lang/Thread/virtual/libSynchronizedNative.c line 29:
> 27: Java_SynchronizedNative_runWithSynchronizedNative(JNIEnv *env, jobject
> obj, jobject task) {
> 28: jclass clazz = (*env)->GetObjectClass(env, obj);
> 29: jmethodID mid = (*env)->GetMethodID(env, clazz, "run",
> "(Ljava/lang/Runnable;)V");
Does this generate a warning with `-Xcheck:jni` due to exceptions not being
checked for?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18600#discussion_r1552648444
