Skip to content
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

8265153: add time based test for ThreadMXBean.getThreadInfo() and ThreadInfo.getLockOwnerName() #3478

Closed
wants to merge 7 commits into from

Conversation

@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Apr 14, 2021

The synopsis pretty much says it all. There's a more detailed history in the RFE itself.

Currently running the new test thru Mach5 Tier[1-7].
I've run the test thru several 12 hour runs on my MBP13 and
on my Linux-X64 server.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8265153: add time based test for ThreadMXBean.getThreadInfo() and ThreadInfo.getLockOwnerName()

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3478/head:pull/3478
$ git checkout pull/3478

Update a local copy of the PR:
$ git checkout pull/3478
$ git pull https://git.openjdk.java.net/jdk pull/3478/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3478

View PR using the GUI difftool:
$ git pr show -t 3478

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3478.diff

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Apr 14, 2021

/label add hotspot-runtime
/label add serviceability

Loading

@dcubed-ojdk dcubed-ojdk marked this pull request as ready for review Apr 14, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 14, 2021

👋 Welcome back dcubed! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 14, 2021

@dcubed-ojdk
The hotspot-runtime label was successfully added.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 14, 2021

@dcubed-ojdk
The serviceability label was successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 14, 2021

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Apr 19, 2021

Ping! Any takers for the new test review?

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Apr 27, 2021

Tap, tap, tap... is this thing working? :-)

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Apr 27, 2021

When reviewing this new test, is it worth comparing it with counter based tests that it was based on, or is it best just to view it as a completely new test.

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Apr 27, 2021

It is not really useful to compare with the counter based M&M tests
because this test is exercising/stressing a different API. It could be
useful to compare this test to runtime/Thread/SuspendAtExit.java
which is a test that was also updated to be time based instead of
counter based.

I am reworking the remaining existing counter based Thread-SMR tests with
a different bug ID (https://bugs.openjdk.java.net/browse/JDK-8266130),
but that is a work-in-progress.

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

HI Dan,

Some minor comments, but to be frank I have no idea what this test is actually doing - sorry.

Cheers,
David

Loading

* @summary The test checks that ThreadInfo.getLockOwnerName() returns a
* non-null string for a blocked thread and then makes repeated calls
* to getThreadInfo() and ThreadInfo.getLockOwnerName() until the thread
* has exited.
Copy link
Member

@dholmes-ora dholmes-ora Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space before has.

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Loading

* has exited.
* @requires vm.jvmti
* @library /test/lib
* @compile getLockOwnerName.java
Copy link
Member

@dholmes-ora dholmes-ora Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need a @compile statement for the current test file

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted.

Loading

}
}

System.exit(run(timeMax, System.out) + exit_delta);
Copy link
Member

@dholmes-ora dholmes-ora Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jtreg tests don't use System.exit!

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... that's generally true, but this is a test that must be run as
"othervm" so this style of exit with the "+ exit_delta" logic has been
used for these kinds of stress tests. I think this style came from the
VMTestbase tests and I've used it with other stress tests.

Loading


while (testState != TS_BLOCKER_RUNNING) {
try {
barrierLaunch.wait(0); // wait until it is running
Copy link
Member

@dholmes-ora dholmes-ora Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: we use wait() rather than wait(0)

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it in 5 places.

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk left a comment

@dholmes-ora - Thanks for the review. Sorry for the delay in responding.
I've been working on other issues. Please see my embedded replies.

Loading

* @summary The test checks that ThreadInfo.getLockOwnerName() returns a
* non-null string for a blocked thread and then makes repeated calls
* to getThreadInfo() and ThreadInfo.getLockOwnerName() until the thread
* has exited.
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Loading

* has exited.
* @requires vm.jvmti
* @library /test/lib
* @compile getLockOwnerName.java
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted.

Loading

}
}

System.exit(run(timeMax, System.out) + exit_delta);
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... that's generally true, but this is a test that must be run as
"othervm" so this style of exit with the "+ exit_delta" logic has been
used for these kinds of stress tests. I think this style came from the
VMTestbase tests and I've used it with other stress tests.

Loading


while (testState != TS_BLOCKER_RUNNING) {
try {
barrierLaunch.wait(0); // wait until it is running
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it in 5 places.

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented May 11, 2021

@dholmes-ora - Sorry I missed this comment:

Some minor comments, but to be frank I have no idea what this test is actually doing - sorry.

This test stresses getThreadInfo() and ThreadInfo.getLockOwnerName() as a
JavaThread is exiting. Similar to other stress tests that I wrote for M&M in the
Thread-SMR project, but this one is targeting getting the lock owner name as
the lock owner thread is exiting.

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented May 11, 2021

@dholmes-ora also sent the comment in email but it didn't seem to
show up in this PR:

Hmmm... this is a legacy style/approach that is not necessary any more.
If the test fails it should throw an exception from the main thread and
that will produce a non-zero exit value. I think seeing this legacy style in
new tests will just confuse people.

I agree. In particular this legacy style approach with exit_delta == 95 is
for VMTestbase tests and this new test is not part of the VMTestbase
test suite. I'm fixing this.

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Dan,

Seems fine. Thanks for the updates.

The proof of a test is in its execution :)

David

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 13, 2021

@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:

8265153: add time based test for ThreadMXBean.getThreadInfo() and ThreadInfo.getLockOwnerName()

Reviewed-by: dholmes, cjplummer

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 198 new commits pushed to the master branch:

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 master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label May 13, 2021
@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented May 13, 2021

@dholmes-ora - Thanks for the re-review. My Mach5 Tier[1-7] testing for an
earlier version of the test is documented in the bug report.

@plummercj - Are you still planning to review this new test?

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented May 13, 2021

@plummercj - Are you still planning to review this new test?

Yes

Loading

Copy link
Contributor

@plummercj plummercj left a comment

Overall it looks good. Some minor suggestions, most of which can be ignored if you wish.

Is there a reason these tests were not placed under test/jdk/java/lang/management?

Loading

// <join returns>
//

public class getLockOwnerName {
Copy link
Contributor

@plummercj plummercj May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be called GetLockOwnerName? We don't usually use lower case for a class name.

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I did that because the API is called:
ThreadInfo.getLockOwnerName()
Will fix the filenames and the classname.

Loading

System.err.println(" -p ::= print debug info");
System.err.println(" time_max ::= max looping time in seconds");
Copy link
Contributor

@plummercj plummercj May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

::= doesn't seem to be a convention we use in help output other than in the other recent tests you've added.

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

Copy link
Contributor

@plummercj plummercj May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention for java tools seems to be to just use tabs to align the start of the argument descriptions:

   -p          print debug info
   time_max    max looping time in seconds

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Loading

getLockOwnerName.logDebug("thread running");

//
// Launch the blocker thread:
Copy link
Contributor

@plummercj plummercj May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Loading

// - holds threadLock until we tell it let go
// - releases threadLock
//
if (getName().equals("blocker")) {
Copy link
Contributor

@plummercj plummercj May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Contributor

@plummercj plummercj May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I get it about lambdas, but they can be useful for simplifying thread tasks without creating a subclass. Here are a few examples, but no need for you to replicate any them:

    // create thread with specified method as the "run" method
    Thread t = new Thread(this::testMethod);
    t.start();
    // create thread with the specified code block as the "run" method
    Thread t1 = new Thread(() -> {
        synchronized (lock1) {
            System.out.println("Thread in sync section 1: "
                    + Thread.currentThread().getName());
            test1();
        }
    // create a static Runnable object using a lambda and use it as the Runnable for a new Thread
    static final Runnable CONSUMER = () -> {
        remove(QUEUE);
    };
    ...
    Thread t = new Thread(CONSUMER);

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll pass on the lambdas.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 1/05/2021 6:57 am, Daniel D.Daugherty wrote:

On Wed, 28 Apr 2021 12:55:18 GMT, David Holmes <dholmes at openjdk.org> wrote:

test/hotspot/jtreg/serviceability/monitoring/ThreadInfo/getLockOwnerName/getLockOwnerName.java line 137:

135: }
136:
137: System.exit(run(timeMax, System.out) + exit_delta);

jtreg tests don't use System.exit!

Hmmm... that's generally true, but this is a test that must be run as
"othervm" so this style of exit with the "+ exit_delta" logic has been
used for these kinds of stress tests. I think this style came from the
VMTestbase tests and I've used it with other stress tests.

Hmmm... this is a legacy style/approach that is not necessary any more.
If the test fails it should throw an exception from the main thread and
that will produce a non-zero exit value. I think seeing this legacy
style in new tests will just confuse people.

YMMV.

Cheers,
David

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 1/05/2021 6:57 am, Daniel D.Daugherty wrote:

On Wed, 28 Apr 2021 12:55:18 GMT, David Holmes <dholmes at openjdk.org> wrote:

test/hotspot/jtreg/serviceability/monitoring/ThreadInfo/getLockOwnerName/getLockOwnerName.java line 137:

135: }
136:
137: System.exit(run(timeMax, System.out) + exit_delta);

jtreg tests don't use System.exit!

Hmmm... that's generally true, but this is a test that must be run as
"othervm" so this style of exit with the "+ exit_delta" logic has been
used for these kinds of stress tests. I think this style came from the
VMTestbase tests and I've used it with other stress tests.

Hmmm... this is a legacy style/approach that is not necessary any more.
If the test fails it should throw an exception from the main thread and
that will produce a non-zero exit value. I think seeing this legacy
style in new tests will just confuse people.

YMMV.

Cheers,
David

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented May 14, 2021

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.

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented May 14, 2021

@plummercj - please mark this as 'Reviewed' if this latest version is acceptable. Thanks.

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented May 15, 2021

@dholmes-ora and @plummercj - Thanks for the reviews.

/integrate

Loading

@openjdk openjdk bot closed this May 15, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 15, 2021

@dcubed-ojdk Since your change was applied there have been 200 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 8c71144.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

@dcubed-ojdk dcubed-ojdk deleted the JDK-8265153 branch May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants