On Fri, 14 May 2021 04:45:36 GMT, Chris Plummer <[email protected]> wrote:
> Overall it looks good. Some minor suggestions, most of which can be ignored
> if you wish.
Thanks for the review. I made some of the changes, but not all of them.
> Is there a reason these tests were not placed under
> test/jdk/java/lang/management?
These tests are for stressing particular HotSpot implementation pieces that are
used
by the M&M APIs. In particular, they are stressing the use of ThreadsListHandles
in both the safepoint and non-safepoint code paths traveled by getThreadInfo():
// maxDepth == 0 requires no safepoint so alternate.
int maxDepth = ((count % 1) == 1) ? Integer.MAX_VALUE : 0;
info = mbean.getThreadInfo(id, maxDepth);
if (info == null) {
// the contender has exited
break;
}
name = info.getLockOwnerName();
And we're stressing getLockOwnerName() because we had a non-reproducible
bug that crashed in that particular function after the getThreadInfo() call.
So I think this stress test (along with others that I've written in this
family) belong
in the test/hotspot collection of tests.
> test/hotspot/jtreg/serviceability/monitoring/ThreadInfo/getLockOwnerName/getLockOwnerName.java
> line 70:
>
>> 68: //
>> 69:
>> 70: public class getLockOwnerName {
>
> Shouldn't this be called GetLockOwnerName? We don't usually use lower case
> for a class name.
Looks like I did that because the API is called:
ThreadInfo.getLockOwnerName()
Will fix the filenames and the classname.
> test/hotspot/jtreg/serviceability/monitoring/ThreadInfo/getLockOwnerName/getLockOwnerName.java
> line 150:
>
>> 148: System.err.println("where:");
>> 149: System.err.println(" -p ::= print debug info");
>> 150: System.err.println(" time_max ::= max looping time in
>> seconds");
>
> `::=` doesn't seem to be a convention we use in help output other than in the
> other recent tests you've added.
It's a grammar notational style from my compiler theory days.
I've used '::=' and ':=' for years. What would you like it changed to?
Or can I just leave it and try to use '-' in the future?
> test/hotspot/jtreg/serviceability/monitoring/ThreadInfo/getLockOwnerName/getLockOwnerName.java
> line 324:
>
>> 322:
>> 323: //
>> 324: // Launch the blocker thread:
>
> The comment says "Launch the blocker thread", but this thread is already
> launched. Maybe drop "Launch" in favor of "Run", or just say "The block
> thread". Same comment for the other two threads.
Fixed.
> test/hotspot/jtreg/serviceability/monitoring/ThreadInfo/getLockOwnerName/getLockOwnerName.java
> line 329:
>
>> 327: // - releases threadLock
>> 328: //
>> 329: if (getName().equals("blocker")) {
>
> Bit of a nit here, but is there a reason not to just create separate Thread
> subclasses for each of the 3 types of threads you are handling here? Or just
> make these each separate static methods of the main class and use the
> instantiate the Thread class with a lambda.
This new test is a variation of a 20 year old test that I recently ported to
JVM/TI
and integrated. 20 years ago, it was much simpler to write the test this way.
I could create a separate Thread subclass for each "role", but I'd rather not
do that since it will no longer be easy to compare this test to its siblings.
As for lambdas, I know absolutely zero about writing lambda code.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3478
