-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8262881: port JVM/DI tests from JDK-4413752 to JVM/TI #2899
Conversation
👋 Welcome back dcubed! A progress list of the required criteria for merging this PR into |
@dcubed-ojdk The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
/label add hotspot-runtime |
@dcubed-ojdk |
These new tests were exercised with Mach5 Tier[134567] testing. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 I'll fix the indents in the .cpp files. Right now I'm using these tests to shake |
|
||
native static int GetResult(); | ||
native static void SetPrintDebug(); | ||
native static void SuspendThread(int id, SuspendWithObjectMonitorEnterWorker thr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I detect that this code is problematic. According to the Bad practice (BAD_PRACTICE), 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Hi Dan, If you change the native methods, e.g. : You can then handle the jvmti error in the Java code, so you don't need to pass down the 'id' of the thread. 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. Thanks, Robbin |
@robehn - Thanks for the review and for the suggested improvements to the tests. |
Hi Dan,
Thank you for making changes! libSuspendWithObjectMonitorEnter.cpp
Thanks, |
@sspitsyn - Thanks for the re-review. I'll take care of the unused variables |
fb564a2
to
d548c7b
Compare
Changes for the next version of the tests:
@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. @lyndseyBeil - I renamed the remaining native methods to @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 I also used the latest version of the tests to reproduce the failure mode that I'm |
@robehn, @lyndseyBeil and @sspitsyn - please re-review when you get the chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like simple agent code 👍
Thanks!
/Robbin
@dcubed-ojdk This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 93 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@robehn - Thanks for the re-review! And thank you for suggesting the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 The I consider @robehn's idea here to be good evolution for these kinds of tests to I hope I have convinced you that this is a good changeset for moving forward. |
Dan, |
@sspitsyn - First, thanks for approving the changes! Second, I'm not proposing changing any other JVM/TI tests to use this style. The only Again, thanks for approving these tests. |
Dan, thank you for extra explanation and for taking care to port these old tests! |
/integrate |
@dcubed-ojdk Since your change was applied there have been 127 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 1ca4abe. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Add three tests from JDK-4413752 ported to JVM/TI:
RawMonitorEnter() with SuspendThread()
ObjectMonitor enter() with SuspendThread()
ObjectMonitor wait() with SuspendThread
The Java files have a transaction diagram to show what each of the
threads in the test is doing.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2899/head:pull/2899
$ git checkout pull/2899
Update a local copy of the PR:
$ git checkout pull/2899
$ git pull https://git.openjdk.java.net/jdk pull/2899/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2899
View PR using the GUI difftool:
$ git pr show -t 2899
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2899.diff