8266130: convert Thread-SMR stress tests from counter based to time based #4237

dcubed-ojdk · 2021-05-28T00:13:25Z

The Thread-SMR project added counter based tests for various APIs.
See "JDK-8167108 inconsistent handling of SR_lock can lead to crashes".

Time based tests are more appropriate for stress kits so I'm
updating the counter based tests to be time based instead.

Two of the updated tests have shaken out failures that are tracked by:

JDK-8264605 vmTestbase/nsk/jvmti/SuspendThread/suspendthrd003/TestDescription.java failed with "agent_tools.cpp, 471: (foundThread = (jthread) jni_env->NewGlobalRef(foundThread)) != NULL"

JDK-8266593 vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be"

These updated tests are tested via Mach5 Tier[134567].
They have also been test by my Stress Kit runs for jdk-17+2[0-5].

Progress

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

Issue

JDK-8266130: convert Thread-SMR stress tests from counter based to time based

Reviewers

Chris Plummer (@plummercj - Reviewer) ⚠️ Review applies to 2ab79a2
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 5c2633d

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4237

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

Using diff file

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

dcubed-ojdk · 2021-05-28T00:14:26Z

/label add hotspot-runtime
/label add serviceability

bridgekeeper · 2021-05-28T00:14:30Z

👋 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.

openjdk · 2021-05-28T00:16:05Z

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

openjdk · 2021-05-28T00:16:59Z

@dcubed-ojdk
The serviceability label was successfully added.

mlbridge · 2021-05-28T21:57:31Z

Webrevs

dcubed-ojdk · 2021-05-29T13:43:13Z

Here are my Mach5 Tier[1-7] results:

Mach5 Tier1:

no failures
25 runs of the updated tests in Tier1

Mach5 Tier2:

1 known, unrelated failure
The updated tests don't run in Tier2.

Mach5 Tier3:

5 known, unrelated test failures
70 runs of the updated tests

Mach5 Tier4:

17 runs of the updated tests
2 known, unrelated test failures

Mach5 Tier5:

56 runs of the updated tests
1 known, unrelated failure, 3 failures of the updated tests linked to the known bugs

Mach5 Tier6:

122 runs of the updated tests
2 failures of the updated tests, linked to the known bugs

Mach5 Tier7:

151 runs of the updated tests
1 failure of the updated tests, linked to the known bug

Because of the failures in the upper tiers, when this PR is integrated I will be ProblemListing:

vmTestbase/nsk/jvmti/SuspendThread/suspendthrd003/TestDescription.java 8264605
vmTestbase/nsk/jvmti/PopFrame/popframe011/TestDescription.java 8266593

The suspendthrd003/8264605 failure had been seen one time in the CI on 2021.04.01
(April Fools Day!) and is now much more reproducible with the updated version. The
popframe011/8266593 failure had never been seen before being updated.

dcubed-ojdk · 2021-06-07T15:17:36Z

Ping! Any takers?
@dholmes-ora and @plummercj - You both reviewed the related test work done in:
JDK-8265153 add time based test for ThreadMXBean.getThreadInfo() and ThreadInfo.getLockOwnerName()
Any chance you have the cycles for these updates this week?

plummercj · 2021-06-07T17:55:17Z

@dholmes-ora and @plummercj - You both reviewed the related test work done in:
JDK-8265153 add time based test for ThreadMXBean.getThreadInfo() and ThreadInfo.getLockOwnerName()
Any chance you have the cycles for these updates this week?

Yes. Hopefully in the next day or two.

plummercj · 2021-06-07T22:09:51Z

test/hotspot/jtreg/runtime/Thread/InterruptAtExit.java

+                thread.startSyncObj.await();
+                // Tell the worker thread to race to the exit and the
+                // Thread.interrupt() calls will come in during thread exit.
+                thread.exitSyncObj.countDown();
+                while (true) {
+                    thread.interrupt();


I just want to make sure I correctly understand the intent here. You call countDown() to unblock the thread's run() method, and then immediately interrupt the thread to create a race between the thread doing a normal exit and the thread being interrupted before or as it exits.

That is correct. The intention is to stress the Thread.interrupt() calls
on the exiting thread.

plummercj · 2021-06-07T22:19:16Z

test/hotspot/jtreg/runtime/Thread/StopAtExit.java

+                // Thread.stop() calls will come in during thread exit.
+                thread.exitSyncObj.countDown();
+                while (true) {
+                    thread.stop();


It seems the main() method of these 6 or so thread tests are identical except for the one Thread API you call at this location. Have you considered refactoring so there is only one copy of all the common code?

Not really. It would be harder to use one of these tests in a standalone
fashion outside of JTREG runs if it depended on another .java file.

plummercj · 2021-06-07T22:38:54Z

test/hotspot/jtreg/vmTestbase/nsk/jvmti/InterruptThread/intrpthrd003.java


-        System.out.println("Case 2: java.lang.Thread.interrupt()");


Was Case 2 removed because it is covered by runtime/Thread/InterruptAtExit.java

Yes, it seemed like duplicated coverage to me.

plummercj · 2021-06-07T23:10:55Z

test/hotspot/jtreg/vmTestbase/nsk/jvmti/SuspendThread/suspendthrd003/suspendthrd003.cpp

-         * Using printf() instead of NSK_DISPLAY1() in this loop
-         * in order to slow down the rate of SuspendThread() calls.


I don't understand the reason for wanting to slow down the rate, nor your reason for removing it.

I couldn't remember why I did that when I wrote the test so
I removed it.

Maybe to allow the resume to get the thread back into real code before the next suspend hits?

openjdk · 2021-06-08T03:56:55Z

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

8266130: convert Thread-SMR stress tests from counter based to time based

Reviewed-by: cjplummer, dholmes

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

f4b3170: 8268428: Test java/foreign/TestResourceScope.java fails: expected [M] but found [N]
6b6ff53: 8268543: some runtime/verifier tests should be run in driver mode
0924382: 8266766: Arrays of types that cannot be an annotation member do not yield exceptions
d43c8a7: 8268124: Update java.lang to use switch expressions
a187fcc: 8238197: JFR: Rework setting and getting EventHandler
f770f77: 8268390: G1 concurrent gc upgrade to full gc not working
e0c0b13: 8268534: some serviceability/jvmti tests should be run in driver mode
92f0b6d: 8268532: several serviceability/attach tests should be run in driver mode
964118f: 8268538: mark hotspot serviceability/logging tests which ignore external VM flags
05090fc: 8268536: mark hotspot serviceability/dcmd tests which ignore external VM flags
... and 225 more: https://git.openjdk.java.net/jdk/compare/8a31c07598cd5ea1305a9706d80b0251fd3a1e6d...master

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.

dcubed-ojdk · 2021-06-08T16:41:56Z

@plummercj - Thanks for the review!

Any other takers out there?

…cription.java until 8264605 is fixed and vmTestbase/nsk/jvmti/PopFrame/popframe011/TestDescription.java until 8266593 is fixed.

dholmes-ora

Hi Dan,

This all seems okay. Sorry it took so long to get to it.

Note: All the usage() calls are indented incorrectly.

Thanks,
David

dholmes-ora · 2021-06-09T01:28:33Z

test/hotspot/jtreg/vmTestbase/nsk/jvmti/SuspendThread/suspendthrd003/suspendthrd003.cpp

-         * Using printf() instead of NSK_DISPLAY1() in this loop
-         * in order to slow down the rate of SuspendThread() calls.


Maybe to allow the resume to get the thread back into real code before the next suspend hits?

dcubed-ojdk · 2021-06-10T12:59:40Z

@dholmes-ora - Thanks for the review! I fixed the indent on the usage() calls.

dcubed-ojdk · 2021-06-10T13:08:32Z

/integrate

openjdk · 2021-06-10T13:09:48Z

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

6c552a7: 8268544: some runtime/sealedClasses tests should be run in driver mode
f4b3170: 8268428: Test java/foreign/TestResourceScope.java fails: expected [M] but found [N]
6b6ff53: 8268543: some runtime/verifier tests should be run in driver mode
0924382: 8266766: Arrays of types that cannot be an annotation member do not yield exceptions
d43c8a7: 8268124: Update java.lang to use switch expressions
a187fcc: 8238197: JFR: Rework setting and getting EventHandler
f770f77: 8268390: G1 concurrent gc upgrade to full gc not working
e0c0b13: 8268534: some serviceability/jvmti tests should be run in driver mode
92f0b6d: 8268532: several serviceability/attach tests should be run in driver mode
964118f: 8268538: mark hotspot serviceability/logging tests which ignore external VM flags
... and 226 more: https://git.openjdk.java.net/jdk/compare/8a31c07598cd5ea1305a9706d80b0251fd3a1e6d...master

Your commit was automatically rebased without conflicts.

Pushed as commit f677163.

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

8266130: covert Thread-SMR stress tests from counter based to time based

33d16ed

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label May 28, 2021

openjdk bot added the serviceability serviceability-dev@openjdk.org label May 28, 2021

Use portable LL format from jni_tools.h for printf() with jlong.

8ec5cae

dcubed-ojdk changed the title ~~8266130: covert Thread-SMR stress tests from counter based to time based~~ 8266130: convert Thread-SMR stress tests from counter based to time based May 28, 2021

dcubed-ojdk marked this pull request as ready for review May 28, 2021 21:50

openjdk bot added the rfr Pull request is ready for review label May 28, 2021

Apply @plummercj code review fix from JDK-8265153 to this review also.

2ab79a2

plummercj reviewed Jun 7, 2021

View reviewed changes

plummercj approved these changes Jun 8, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 8, 2021

ProblemList vmTestbase/nsk/jvmti/SuspendThread/suspendthrd003/TestDes…

5c2633d

…cription.java until 8264605 is fixed and vmTestbase/nsk/jvmti/PopFrame/popframe011/TestDescription.java until 8266593 is fixed.

dholmes-ora approved these changes Jun 9, 2021

View reviewed changes

dholmes CR - fix indent on usage() calls.

2daef5a

openjdk bot closed this Jun 10, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 10, 2021

dcubed-ojdk deleted the JDK-8266130 branch June 10, 2021 19:10

		* Using printf() instead of NSK_DISPLAY1() in this loop
		* in order to slow down the rate of SuspendThread() calls.

8266130: convert Thread-SMR stress tests from counter based to time based #4237

8266130: convert Thread-SMR stress tests from counter based to time based #4237

Uh oh!

Conversation

dcubed-ojdk commented May 28, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

dcubed-ojdk commented May 28, 2021

Uh oh!

bridgekeeper bot commented May 28, 2021

Uh oh!

openjdk bot commented May 28, 2021

Uh oh!

openjdk bot commented May 28, 2021

Uh oh!

mlbridge bot commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dcubed-ojdk commented May 29, 2021

Uh oh!

dcubed-ojdk commented Jun 7, 2021

Uh oh!

plummercj commented Jun 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcubed-ojdk commented Jun 8, 2021

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcubed-ojdk commented Jun 10, 2021

Uh oh!

dcubed-ojdk commented Jun 10, 2021

Uh oh!

openjdk bot commented Jun 10, 2021

Uh oh!

Uh oh!

dcubed-ojdk commented May 28, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented May 28, 2021 •

edited

Loading

openjdk bot commented Jun 8, 2021 •

edited

Loading