RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI

Daniel D.Daugherty
Add three tests from JDK-4413752 ported to JVM/TI:

- RawMonitorEnter() with SuspendThread()
  - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/SuspendWithRawMonitorEnter.java
  - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/libSuspendWithRawMonitorEnter.cpp

- ObjectMonitor enter() with SuspendThread()
  - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java
  - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/libSuspendWithObjectMonitorEnter.cpp

- ObjectMonitor wait() with SuspendThread
  - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
  - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/libSuspendWithObjectMonitorWait.cpp

The Java files have a transaction diagram to show what each of the
threads in the test is doing.

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

Commit messages:
 - 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI

Changes: https://git.openjdk.java.net/jdk/pull/2899/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2899&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262881
  Stats: 1599 lines in 6 files changed: 1599 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2899.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2899/head:pull/2899

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI

Daniel D.Daugherty
On Tue, 9 Mar 2021 21:08:54 GMT, Daniel D. Daugherty <[hidden email]> wrote:

> Add three tests from JDK-4413752 ported to JVM/TI:
>
> - RawMonitorEnter() with SuspendThread()
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/SuspendWithRawMonitorEnter.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/libSuspendWithRawMonitorEnter.cpp
>
> - ObjectMonitor enter() with SuspendThread()
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/libSuspendWithObjectMonitorEnter.cpp
>
> - ObjectMonitor wait() with SuspendThread
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/libSuspendWithObjectMonitorWait.cpp
>
> The Java files have a transaction diagram to show what each of the
> threads in the test is doing.

These new tests were exercised with Mach5 Tier[134567] testing.
They have also been tested on macOSX and Linux-X64 with the three
build configs running in parallel: fastdebug, release, slowdebug for
each of the three new tests for 6 hours. The results for macOS and
Linux-X64 are in the bug report.

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI

Serguei Spitsyn-2
In reply to this post by Daniel D.Daugherty
On Tue, 9 Mar 2021 21:08:54 GMT, Daniel D. Daugherty <[hidden email]> wrote:

> Add three tests from JDK-4413752 ported to JVM/TI:
>
> - RawMonitorEnter() with SuspendThread()
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/SuspendWithRawMonitorEnter.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/libSuspendWithRawMonitorEnter.cpp
>
> - ObjectMonitor enter() with SuspendThread()
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/libSuspendWithObjectMonitorEnter.cpp
>
> - ObjectMonitor wait() with SuspendThread
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/libSuspendWithObjectMonitorWait.cpp
>
> The Java files have a transaction diagram to show what each of the
> threads in the test is doing.

Hi Dan,
It is interesting how much these tests were changed when ported.
A couple of indent-related comments.
There are incorrect indents in the following lines in all .cpp files:
 ```
68 Java_SuspendWithObjectMonitorEnter_GetResult(JNIEnv *env, jclass cls) {
 69     return iGlobalStatus;
 70 }
  72 JNIEXPORT void JNICALL
 73 Java_SuspendWithObjectMonitorEnter_SetPrintDebug(JNIEnv *env, jclass cls) {
 74     printdebug = 1;
 75 }
 97 Java_SuspendWithObjectMonitorEnterWorker_GetPrintDebug(JNIEnv *env, jclass cls) {
 98     return printdebug;
 99 }

Thanks,
Serguei

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI

Daniel D.Daugherty
On Wed, 17 Mar 2021 07:18:07 GMT, Serguei Spitsyn <[hidden email]> wrote:

>> Add three tests from JDK-4413752 ported to JVM/TI:
>>
>> - RawMonitorEnter() with SuspendThread()
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/SuspendWithRawMonitorEnter.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/libSuspendWithRawMonitorEnter.cpp
>>
>> - ObjectMonitor enter() with SuspendThread()
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/libSuspendWithObjectMonitorEnter.cpp
>>
>> - ObjectMonitor wait() with SuspendThread
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/libSuspendWithObjectMonitorWait.cpp
>>
>> The Java files have a transaction diagram to show what each of the
>> threads in the test is doing.
>
> Hi Dan,
> It is interesting how much these tests were changed when ported.
> A couple of indent-related comments.
> There are incorrect indents in the following lines in all .cpp files:
>  ```
> 68 Java_SuspendWithObjectMonitorEnter_GetResult(JNIEnv *env, jclass cls) {
>  69     return iGlobalStatus;
>  70 }
>   72 JNIEXPORT void JNICALL
>  73 Java_SuspendWithObjectMonitorEnter_SetPrintDebug(JNIEnv *env, jclass cls) {
>  74     printdebug = 1;
>  75 }
>  97 Java_SuspendWithObjectMonitorEnterWorker_GetPrintDebug(JNIEnv *env, jclass cls) {
>  98     return printdebug;
>  99 }
>
> Thanks,
> Serguei

@sspitsyn - Thanks for the review! I figured you would enjoy this 20 year old
blast from the past! I'm tempted to ping @karenkinnear just to see if she'll
remember these tests!

I'll fix the indents in the .cpp files. Right now I'm using these tests to shake
out @robehn's work on "JDK-8257831 Suspend with handshakes".

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v2]

Daniel D.Daugherty
In reply to this post by Daniel D.Daugherty
> Add three tests from JDK-4413752 ported to JVM/TI:
>
> - RawMonitorEnter() with SuspendThread()
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/SuspendWithRawMonitorEnter.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/libSuspendWithRawMonitorEnter.cpp
>
> - ObjectMonitor enter() with SuspendThread()
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/libSuspendWithObjectMonitorEnter.cpp
>
> - ObjectMonitor wait() with SuspendThread
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/libSuspendWithObjectMonitorWait.cpp
>
> The Java files have a transaction diagram to show what each of the
> threads in the test is doing.

Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:

  sspitsyn - fix white space indents problems.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2899/files
  - new: https://git.openjdk.java.net/jdk/pull/2899/files/3e719713..fb564a25

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2899&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2899&range=00-01

  Stats: 9 lines in 3 files changed: 0 ins; 0 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2899.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2899/head:pull/2899

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v2]

Robbin Ehn-2
In reply to this post by Daniel D.Daugherty
On Fri, 19 Mar 2021 20:16:29 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Hi Dan,
>> It is interesting how much these tests were changed when ported.
>> A couple of indent-related comments.
>> There are incorrect indents in the following lines in all .cpp files:
>>  ```
>> 68 Java_SuspendWithObjectMonitorEnter_GetResult(JNIEnv *env, jclass cls) {
>>  69     return iGlobalStatus;
>>  70 }
>>   72 JNIEXPORT void JNICALL
>>  73 Java_SuspendWithObjectMonitorEnter_SetPrintDebug(JNIEnv *env, jclass cls) {
>>  74     printdebug = 1;
>>  75 }
>>  97 Java_SuspendWithObjectMonitorEnterWorker_GetPrintDebug(JNIEnv *env, jclass cls) {
>>  98     return printdebug;
>>  99 }
>>
>> Thanks,
>> Serguei
>
> @sspitsyn - Thanks for the review! I figured you would enjoy this 20 year old
> blast from the past! I'm tempted to ping @karenkinnear just to see if she'll
> remember these tests!
>
> I'll fix the indents in the .cpp files. Right now I'm using these tests to shake
> out @robehn's work on "JDK-8257831 Suspend with handshakes".

Hi Dan,

If you change the native methods, e.g. :
native static void RawMonitorEnter(int id);
To something like:
native static int RawMonitorEnter();

You can then handle the jvmti error in the Java code, so you don't need to pass down the 'id' of the thread.
You then remove all debug code from the C-code agent, which removes some native methods also.
Which also leads to that you don't need  the thread 'id', instead you can just use the thread name, which removes some Java code.

Also you shouldn't catch the UnsatisfiedLinkError, as docs also says: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."

You also have a lot of code for argument parsing which is not used by the test inside the test methods.
Can you either remove that or if you want it put it in a separate class/method, so e.g. "doWork()" takes parsed arguments instead.

Thanks, Robbin

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v2]

Daniel D.Daugherty
On Mon, 22 Mar 2021 09:02:02 GMT, Robbin Ehn <[hidden email]> wrote:

>> @sspitsyn - Thanks for the review! I figured you would enjoy this 20 year old
>> blast from the past! I'm tempted to ping @karenkinnear just to see if she'll
>> remember these tests!
>>
>> I'll fix the indents in the .cpp files. Right now I'm using these tests to shake
>> out @robehn's work on "JDK-8257831 Suspend with handshakes".
>
> Hi Dan,
>
> If you change the native methods, e.g. :
> native static void RawMonitorEnter(int id);
> To something like:
> native static int RawMonitorEnter();
>
> You can then handle the jvmti error in the Java code, so you don't need to pass down the 'id' of the thread.
> You then remove all debug code from the C-code agent, which removes some native methods also.
> Which also leads to that you don't need  the thread 'id', instead you can just use the thread name, which removes some Java code.
>
> Also you shouldn't catch the UnsatisfiedLinkError, as docs also says: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."
>
> You also have a lot of code for argument parsing which is not used by the test inside the test methods.
> Can you either remove that or if you want it put it in a separate class/method, so e.g. "doWork()" takes parsed arguments instead.
>
> Thanks, Robbin

@robehn - Thanks for the review and for the suggested improvements to the tests.
It will take me a bit of time to make and test these suggestions.

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v2]

Serguei Spitsyn-2
On Mon, 22 Mar 2021 14:22:36 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Hi Dan,
>>
>> If you change the native methods, e.g. :
>> native static void RawMonitorEnter(int id);
>> To something like:
>> native static int RawMonitorEnter();
>>
>> You can then handle the jvmti error in the Java code, so you don't need to pass down the 'id' of the thread.
>> You then remove all debug code from the C-code agent, which removes some native methods also.
>> Which also leads to that you don't need  the thread 'id', instead you can just use the thread name, which removes some Java code.
>>
>> Also you shouldn't catch the UnsatisfiedLinkError, as docs also says: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."
>>
>> You also have a lot of code for argument parsing which is not used by the test inside the test methods.
>> Can you either remove that or if you want it put it in a separate class/method, so e.g. "doWork()" takes parsed arguments instead.
>>
>> Thanks, Robbin
>
> @robehn - Thanks for the review and for the suggested improvements to the tests.
> It will take me a bit of time to make and test these suggestions.

Hi Dan,
> I figured you would enjoy this 20 year old blast from the past!
Yes, of course, but these tests look like they have been written today! :)

Thank you for making changes!
I've just noticed one minor issue:

libSuspendWithObjectMonitorEnter.cpp
libSuspendWithObjectMonitorWait.cpp:
The static variables below are not used and can be removed:
 32 static jrawMonitorID threadLock = NULL;
 33 static char threadLockName[] = "threadLock";

Thanks,
Serguei

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v2]

lyndseyBeil
In reply to this post by Daniel D.Daugherty
On Fri, 19 Mar 2021 21:28:09 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Add three tests from JDK-4413752 ported to JVM/TI:
>>
>> - RawMonitorEnter() with SuspendThread()
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/SuspendWithRawMonitorEnter.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/libSuspendWithRawMonitorEnter.cpp
>>
>> - ObjectMonitor enter() with SuspendThread()
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/libSuspendWithObjectMonitorEnter.cpp
>>
>> - ObjectMonitor wait() with SuspendThread
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/libSuspendWithObjectMonitorWait.cpp
>>
>> The Java files have a transaction diagram to show what each of the
>> threads in the test is doing.
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>
>   sspitsyn - fix white space indents problems.

test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java line 92:

> 90:     native static int GetResult();
> 91:     native static void SetPrintDebug();
> 92:     native static void SuspendThread(int id, SuspendWithObjectMonitorEnterWorker thr);

I detect that this code is problematic. According to the [Bad practice (BAD_PRACTICE)](https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#bad-practice-bad-practice), [Nm: Method names should start with a lower case letter (NM_METHOD_NAMING_CONVENTION)](https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#nm-method-names-should-start-with-a-lower-case-letter-nm-method-naming-convention).
Methods should be verbs, in mixed case with the first letter lowercase, with the first letter of each internal word capitalized.

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v2]

Daniel D.Daugherty
On Sun, 21 Mar 2021 15:15:54 GMT, lyndseyBeil <[hidden email]> wrote:

>> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   sspitsyn - fix white space indents problems.
>
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java line 92:
>
>> 90:     native static int GetResult();
>> 91:     native static void SetPrintDebug();
>> 92:     native static void SuspendThread(int id, SuspendWithObjectMonitorEnterWorker thr);
>
> I detect that this code is problematic. According to the [Bad practice (BAD_PRACTICE)](https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#bad-practice-bad-practice), [Nm: Method names should start with a lower case letter (NM_METHOD_NAMING_CONVENTION)](https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#nm-method-names-should-start-with-a-lower-case-letter-nm-method-naming-convention).
> Methods should be verbs, in mixed case with the first letter lowercase, with the first letter of each internal word capitalized.

Nice catch. In the original 20 year old test those methods were:
    native static int diobj4413752GetMonitorInfo(int id, Object mon);
    native static int diobj4413752GetResult();
    native static int diobj4413752SuspendThread(int id, diobjWorker thr);
    native static int diobj4413752Wait4ContendedEnter(int id, diobjWorker thr);
so when I got rid of the `diobj4413752` prefix I forgot to fix the new first letter.
I'll take care of those method renames and I'll double check for others.

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v3]

Daniel D.Daugherty
In reply to this post by Daniel D.Daugherty
> Add three tests from JDK-4413752 ported to JVM/TI:
>
> - RawMonitorEnter() with SuspendThread()
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/SuspendWithRawMonitorEnter.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/libSuspendWithRawMonitorEnter.cpp
>
> - ObjectMonitor enter() with SuspendThread()
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/libSuspendWithObjectMonitorEnter.cpp
>
> - ObjectMonitor wait() with SuspendThread
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/libSuspendWithObjectMonitorWait.cpp
>
> The Java files have a transaction diagram to show what each of the
> threads in the test is doing.

Daniel D. Daugherty has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:

 - Merge branch 'master' into JDK-8262881
 - sspitsyn - fix white space indents problems.
 - 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI

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

Changes: https://git.openjdk.java.net/jdk/pull/2899/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2899&range=02
  Stats: 1599 lines in 6 files changed: 1599 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2899.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2899/head:pull/2899

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v4]

Daniel D.Daugherty
In reply to this post by Daniel D.Daugherty
> Add three tests from JDK-4413752 ported to JVM/TI:
>
> - RawMonitorEnter() with SuspendThread()
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/SuspendWithRawMonitorEnter.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/libSuspendWithRawMonitorEnter.cpp
>
> - ObjectMonitor enter() with SuspendThread()
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/libSuspendWithObjectMonitorEnter.cpp
>
> - ObjectMonitor wait() with SuspendThread
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/libSuspendWithObjectMonitorWait.cpp
>
> The Java files have a transaction diagram to show what each of the
> threads in the test is doing.

Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:

  Address lyndseyBeil, robehn and sspitsyn CR comments.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2899/files
  - new: https://git.openjdk.java.net/jdk/pull/2899/files/d548c7b5..28ac7069

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2899&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2899&range=02-03

  Stats: 640 lines in 6 files changed: 211 ins; 309 del; 120 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2899.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2899/head:pull/2899

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v4]

Daniel D.Daugherty
In reply to this post by Robbin Ehn-2
On Mon, 22 Mar 2021 09:02:02 GMT, Robbin Ehn <[hidden email]> wrote:

>> @sspitsyn - Thanks for the review! I figured you would enjoy this 20 year old
>> blast from the past! I'm tempted to ping @karenkinnear just to see if she'll
>> remember these tests!
>>
>> I'll fix the indents in the .cpp files. Right now I'm using these tests to shake
>> out @robehn's work on "JDK-8257831 Suspend with handshakes".
>
> Hi Dan,
>
> If you change the native methods, e.g. :
> native static void RawMonitorEnter(int id);
> To something like:
> native static int RawMonitorEnter();
>
> You can then handle the jvmti error in the Java code, so you don't need to pass down the 'id' of the thread.
> You then remove all debug code from the C-code agent, which removes some native methods also.
> Which also leads to that you don't need  the thread 'id', instead you can just use the thread name, which removes some Java code.
>
> Also you shouldn't catch the UnsatisfiedLinkError, as docs also says: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."
>
> You also have a lot of code for argument parsing which is not used by the test inside the test methods.
> Can you either remove that or if you want it put it in a separate class/method, so e.g. "doWork()" takes parsed arguments instead.
>
> Thanks, Robbin

@robehn, @lyndseyBeil and @sspitsyn - please re-review when you get the chance.

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v4]

Daniel D.Daugherty
In reply to this post by Serguei Spitsyn-2
On Tue, 23 Mar 2021 14:10:29 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Hi Dan,
>>> I figured you would enjoy this 20 year old blast from the past!
>> Yes, of course, but these tests look like they have been written today! :)
>>
>> Thank you for making changes!
>> I've just noticed one minor issue:
>>
>> libSuspendWithObjectMonitorEnter.cpp
>> libSuspendWithObjectMonitorWait.cpp:
>> The static variables below are not used and can be removed:
>>  32 static jrawMonitorID threadLock = NULL;
>>  33 static char threadLockName[] = "threadLock";
>>
>> Thanks,
>> Serguei
>
> @sspitsyn - Thanks for the re-review. I'll take care of the unused variables
> and I'll do an audit of all three tests and look for more.

Changes for the next version of the tests:

- @robehn CR changes:
    -  changed the JVM/TI function wrappers to be much simpler and just return the JVM/TI return code to the Java code caller; all error checking is now on the Java side of the test.
    - dropped the 'id' parameter; deleted many native support functions.

@robehn - I kept the catch of UnsatisfiedLinkError because what I'm doing there is printing a nice error message and then rethrowing the same exception; it makes it easier to debug the build process for the test.
@robehn - I moved the argument parsing code to the main() method; while the default configuration of the test doesn't use command line arguments, I have stress wrappers for these tests that use the command line args.

@lyndseyBeil - I renamed the remaining native methods to `camelCase()` style.

@sspitsyn - I've removed the unused variables from the three tests.

@robehn, @lyndseyBeil and @sspitsyn - thanks for your reviews! New commit coming shortly.

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v4]

Daniel D.Daugherty
On Thu, 1 Apr 2021 19:39:07 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> @sspitsyn - Thanks for the re-review. I'll take care of the unused variables
>> and I'll do an audit of all three tests and look for more.
>
> Changes for the next version of the tests:
>
> - @robehn CR changes:
>     -  changed the JVM/TI function wrappers to be much simpler and just return the JVM/TI return code to the Java code caller; all error checking is now on the Java side of the test.
>     - dropped the 'id' parameter; deleted many native support functions.
>
> @robehn - I kept the catch of UnsatisfiedLinkError because what I'm doing there is printing a nice error message and then rethrowing the same exception; it makes it easier to debug the build process for the test.
> @robehn - I moved the argument parsing code to the main() method; while the default configuration of the test doesn't use command line arguments, I have stress wrappers for these tests that use the command line args.
>
> @lyndseyBeil - I renamed the remaining native methods to `camelCase()` style.
>
> @sspitsyn - I've removed the unused variables from the three tests.
>
> @robehn, @lyndseyBeil and @sspitsyn - thanks for your reviews! New commit coming shortly.

The v03 version was tested with Mach5 Tier[134567] testing. The three new tests passed
in all configurations in all of those tiers.

I also used the latest version of the tests to reproduce the failure mode that I'm
hunting in "JDK-8264393 JDK-8258284 introduced dangling TLH race" so these
tests still help reproduce that bug.

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v4]

Robbin Ehn-2
In reply to this post by Daniel D.Daugherty
On Thu, 1 Apr 2021 19:46:38 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Add three tests from JDK-4413752 ported to JVM/TI:
>>
>> - RawMonitorEnter() with SuspendThread()
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/SuspendWithRawMonitorEnter.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/libSuspendWithRawMonitorEnter.cpp
>>
>> - ObjectMonitor enter() with SuspendThread()
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/libSuspendWithObjectMonitorEnter.cpp
>>
>> - ObjectMonitor wait() with SuspendThread
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/libSuspendWithObjectMonitorWait.cpp
>>
>> The Java files have a transaction diagram to show what each of the
>> threads in the test is doing.
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>
>   Address lyndseyBeil, robehn and sspitsyn CR comments.

I really like simple agent code 👍

Thanks!

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

Marked as reviewed by rehn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v4]

Daniel D.Daugherty
On Wed, 7 Apr 2021 11:43:15 GMT, Robbin Ehn <[hidden email]> wrote:

>> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Address lyndseyBeil, robehn and sspitsyn CR comments.
>
> I really like simple agent code 👍
>
> Thanks!

@robehn - Thanks for the re-review! And thank you for suggesting the
much simpler agent code!!

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v4]

Serguei Spitsyn-2
In reply to this post by Daniel D.Daugherty
On Thu, 1 Apr 2021 19:46:38 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Add three tests from JDK-4413752 ported to JVM/TI:
>>
>> - RawMonitorEnter() with SuspendThread()
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/SuspendWithRawMonitorEnter.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithRawMonitorEnter/libSuspendWithRawMonitorEnter.cpp
>>
>> - ObjectMonitor enter() with SuspendThread()
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/SuspendWithObjectMonitorEnter.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorEnter/libSuspendWithObjectMonitorEnter.cpp
>>
>> - ObjectMonitor wait() with SuspendThread
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>>   - test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/libSuspendWithObjectMonitorWait.cpp
>>
>> The Java files have a transaction diagram to show what each of the
>> threads in the test is doing.
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>
>   Address lyndseyBeil, robehn and sspitsyn CR comments.

Hi Dan,
It looks good in general.
But I think moving JVMTI error code checks to Java is a step in a wrong direction.
First, it makes it inconsistent with all existing JVMTI tests. In this particular case, all the complexity is moved to Java side making it unbalanced. Also, we are working with Leonid toward creating relevant native testing lib for serviceability/jvmti tests (created it for Loom first) which has a support to print symbolic names (for error codes as well) if necessary.
Not sure, I understood what wrong with the native check_jvmti_status(). It seems the id argument is not needed much and can be removed anyway.
Thanks,
Serguei

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

Marked as reviewed by sspitsyn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v4]

Daniel D.Daugherty
On Wed, 7 Apr 2021 17:52:16 GMT, Serguei Spitsyn <[hidden email]> wrote:

>> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Address lyndseyBeil, robehn and sspitsyn CR comments.
>
> Hi Dan,
> It looks good in general.
> But I think moving JVMTI error code checks to Java is a step in a wrong direction.
> First, it makes it inconsistent with all existing JVMTI tests. In this particular case, all the complexity is moved to Java side making it unbalanced. Also, we are working with Leonid toward creating relevant native testing lib for serviceability/jvmti tests (created it for Loom first) which has a support to print symbolic names (for error codes as well) if necessary.
> Not sure, I understood what wrong with the native check_jvmti_status(). It seems the id argument is not needed much and can be removed anyway.
> Thanks,
> Serguei

Hi @sspitsyn,

I rather like the much simpler agent code and it puts the majority of logic
in the .java file so the reader/reviewer doesn't have to jump back and forth
between the two files nearly as much.

The `id` argument had to be passed to the old version of the function calls
in order for the logging messages generated by the agent code to (easily)
have the same thread names as the Java side logging. Once all the error
checking was moved to the Java side, that made the `id` argument no longer
necessary and also allowed the native `check_jvmti_status()` function (along
with other functions) to be removed. So there was nothing wrong with the
native `check_jvmti_status()` function; it was just made obsolete by the code
migration to the Java side.

I consider @robehn's idea here to be good evolution for these kinds of tests to
move code from the native side, where the code is more platform dependent,
to the Java side, where the code is more platform independent.

I hope I have convinced you that this is a good changeset for moving forward.

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

PR: https://git.openjdk.java.net/jdk/pull/2899
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v4]

Serguei Spitsyn-2
On Wed, 7 Apr 2021 19:17:33 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Hi Dan,
>> It looks good in general.
>> But I think moving JVMTI error code checks to Java is a step in a wrong direction.
>> First, it makes it inconsistent with all existing JVMTI tests. In this particular case, all the complexity is moved to Java side making it unbalanced. Also, we are working with Leonid toward creating relevant native testing lib for serviceability/jvmti tests (created it for Loom first) which has a support to print symbolic names (for error codes as well) if necessary.
>> Not sure, I understood what wrong with the native check_jvmti_status(). It seems the id argument is not needed much and can be removed anyway.
>> Thanks,
>> Serguei
>
> Hi @sspitsyn,
>
> I rather like the much simpler agent code and it puts the majority of logic
> in the .java file so the reader/reviewer doesn't have to jump back and forth
> between the two files nearly as much.
>
> The `id` argument had to be passed to the old version of the function calls
> in order for the logging messages generated by the agent code to (easily)
> have the same thread names as the Java side logging. Once all the error
> checking was moved to the Java side, that made the `id` argument no longer
> necessary and also allowed the native `check_jvmti_status()` function (along
> with other functions) to be removed. So there was nothing wrong with the
> native `check_jvmti_status()` function; it was just made obsolete by the code
> migration to the Java side.
>
> I consider @robehn's idea here to be good evolution for these kinds of tests to
> move code from the native side, where the code is more platform dependent,
> to the Java side, where the code is more platform independent.
>
> I hope I have convinced you that this is a good changeset for moving forward.

Dan,
I'm sorry, I do not see much value in this approach going forward. One line with a call to check_jvmti_status() does not add much to the platform dependency no to complexity in native code. Also, more sophisticated analysis of the returned error code frequently is needed. Also, I consider it not feasible to move a thousand of JVMTI tests in this direction. So that we will end up with inconsistency over all tests.
I've already marked this as reviewed, so you can push it. But I'm against following this pattern going forward. Or at least, we need to consult with the SQE engineers first.
Thanks,
Serguei

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

PR: https://git.openjdk.java.net/jdk/pull/2899
12