RFR: 8265153: add time based test for ThreadMXBean.getThreadInfo() and ThreadInfo.getLockOwnerName() [v5]

[Daniel D.Daugherty]

On Fri, 14 May 2021 04:45:36 GMT, Chris Plummer <cjplummer at openjdk.org> 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