8265240: runtime/Thread/SuspendAtExit.java needs updating #3576

dcubed-ojdk · 2021-04-19T19:15:18Z

I'm updating the runtime/Thread/SuspendAtExit.java test:

switch from java.lang.Thread.suspend() to JVM/TI SuspendThread()
switch from java.lang.Thread.resume() to JVM/TI ResumeThread()
switch from counter-based to time-based testing
improve error checking since we're now using an API with error codes!

I've used this test to stress @robehn's fix for JDK-8257831 using both
invocation styles for 9 hours each in {fastdebug, release, slowdebug}
configs without any issues.

I've run the updated test thru Mach5 Tier[134567] testing; one timeout
was observed in a single Tier6 run on Win-X64. I believe this is a case of
a lost Thread.interrupt() call.

Progress

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

Issue

JDK-8265240: runtime/Thread/SuspendAtExit.java needs updating

Reviewers

Robbin Ehn (@robehn - Reviewer) ⚠️ Review applies to 8235e5f
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3576

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

Using diff file

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

dcubed-ojdk · 2021-04-19T19:15:31Z

/label add hotspot-runtime

bridgekeeper · 2021-04-19T19:15:37Z

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

dcubed-ojdk · 2021-04-19T19:15:45Z

/label add serviceability

openjdk · 2021-04-19T19:16:40Z

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

openjdk · 2021-04-19T19:17:56Z

@dcubed-ojdk
The serviceability label was successfully added.

robehn

Thanks for fixing. Oh only in draft state ;)

openjdk · 2021-04-20T07:01:27Z

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

8265240: runtime/Thread/SuspendAtExit.java needs updating

Reviewed-by: rehn, 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 98 new commits pushed to the master branch:

7a55914: 8264196: Change link_and_cleanup_shared_classes(CATCH) to CHECK
b84f690: 8265793: Remove duplicate jtreg TEST.groups references for some client tests
0e00598: 8265782: Bump bootjdk to jdk-17+19 on macosx-aarch64 at Oracle
e81baea: 8265786: ProblemList serviceability/sa/sadebugd/DisableRegistryTest.java on ZGC
ca0de26: 8265699: (bf) Scopes passed to ScopedMemoryAccess.copy[Swap]Memory in incorrect order
b930bb1: 8265461: G1: Forwarding pointer removal thread sizing
f834557: 8258915: Temporary buffer cleanup
31d8a19: 8265105: gc/arguments/TestSelectDefaultGC.java fails when compiler1 is disabled
657f103: 8057543: Replace javac's Filter with Predicate (and lambdas)
8758b55: 8265591: Remove vestiages of intermediate JSR 175 annotation format
... and 88 more: https://git.openjdk.java.net/jdk/compare/c108e7abdf25c7371cc23c07ea4e584efd33a73e...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-04-20T15:44:30Z

Oops. Forgot to mark it ready for review.

mlbridge · 2021-04-20T15:49:29Z

Webrevs

dholmes-ora

Hi Dan,

A few nits (mostly pre-existing) but otherwise the conversion to JVMTI looks good.

Thanks,
David

dholmes-ora · 2021-04-21T06:03:46Z

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

+        long start_time = System.currentTimeMillis();
+        while (System.currentTimeMillis() < start_time + (timeMax * 1000)) {
+            count++;
+            SuspendAtExit threads[] = new SuspendAtExit[N_THREADS];


Style nit pre-existing: The [] go on the type not the variable.

Good catch!

As I'm re-reading this code, it occurs to me that I really don't need to
create the array of N_THREADS any more since we are now time
based and we'll create a new Thread in each loop and run it through
the gauntlet until times runs out.

What do you think about changing SuspendAtExit[N_THREADS]
into just a single SuspendAtExit?

dholmes-ora · 2021-04-21T06:03:46Z

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

+                if (threads[i].isAlive()) {
+                    throw new RuntimeException("Expected !Thread.isAlive() " +
+                                               "after thread #" + i +
+                                               " has been join()'ed");
+                }


This is unnecessary, you aren't testing the correctness of Thread.join() here. join() can't return normally if the thread is alive.

Okay. Will delete the "late" isAlive() check.

dholmes-ora · 2021-04-21T06:03:46Z

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

+                    // This interrupt() call will break the worker out of
+                    // the exitSyncObj.await() call and the SuspendThread()
+                    // calls will come in during thread exit.
+                    threads[i].interrupt();


Pre-existing: why use interrupt() instead of simply calling countDown() on the latch ??

Hmmm.... I don't remember why I chose to use Thread.interrupt() instead
of countDown() when I created these tests for the Thread-SMR project.
I'll try switching.

dholmes-ora · 2021-04-21T06:03:46Z

test/hotspot/jtreg/runtime/Thread/libSuspendAtExit.cpp

@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2001, 2021, Oracle and/or its affiliates. All rights reserved.


Copyright year should just be 2021.

I copied this code from another file that is "Copyright (c) 2001, 2021"
and renamed just the prefix for the suspendThread() and resumeThread()
functions. Agent_OnLoad() is a straight copy.

So I think I should keep the copyright as is.

Okay. I have no idea what is supposed to happen in such cases.

dholmes-ora · 2021-04-22T02:37:16Z

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

-    final static int N_LATE_CALLS = 10000;
+    private final static String AGENT_LIB = "SuspendAtExit";
+    private final static int DEF_TIME_MAX = 30;  // default max # secs to test
+    private final static int N_THREADS = 32;


N_THREADS is unused now.

Meant to delete that! Will fix it.

dholmes-ora · 2021-04-22T02:40:12Z

I managed to lose my review comment when switching between commits :) so I'll add it here.

Updates look good. I agree there is no need for the Thread[] any more.

Thanks,
David

dcubed-ojdk · 2021-04-23T14:19:13Z

/integrate

openjdk · 2021-04-23T14:20:55Z

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

891f72f: 8265606: Reduce allocations in AdapterHandlerLibrary::get_adapter
bfc1cd8: 8264320: ShouldNotReachHere in Compile::print_inlining_move_to()
5db64c3: 8265268: Unify ReservedSpace reservation code in initialize and try_reserve_heap
191f1fc: 8265759: Shenandoah: Avoid race for referent in assert
b3a319c: 8264842: IGV: different nodes sharing idx are treated as equal
95f0fd6: 8265696: Move CDS sources to src/hotspot/shared/cds
a715b09: 8265798: Minimal build broken by JDK-8261090
13d3263: 8265484: Fix up TRAPS usage in GenerateOopMap::compute_map and callers
a8ddbd1: 8265683: vmTestbase/nsk/jdb tests failed with "JDWP exit error AGENT_ERROR_INTERNAL(181)"
7a55914: 8264196: Change link_and_cleanup_shared_classes(CATCH) to CHECK
... and 97 more: https://git.openjdk.java.net/jdk/compare/c108e7abdf25c7371cc23c07ea4e584efd33a73e...master

Your commit was automatically rebased without conflicts.

Pushed as commit c9b70c8.

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

8265240: runtime/Thread/SuspendAtExit.java needs updating

ed352ea

openjdk bot added the hotspot-runtime label Apr 19, 2021

openjdk bot added the serviceability label Apr 19, 2021

robehn approved these changes Apr 20, 2021

View changes

openjdk bot added the ready label Apr 20, 2021

dcubed-ojdk marked this pull request as ready for review Apr 20, 2021

openjdk bot added the rfr label Apr 20, 2021

dholmes-ora approved these changes Apr 21, 2021

View changes

dholmes CR changes.

8235e5f

dholmes-ora approved these changes Apr 22, 2021

View changes

robehn approved these changes Apr 22, 2021

View changes

dholmes CR - delete unused N_THREADS.

a4aac0d

dholmes-ora approved these changes Apr 22, 2021

View changes

openjdk bot closed this Apr 23, 2021

openjdk bot added integrated and removed ready rfr labels Apr 23, 2021

dcubed-ojdk deleted the JDK-8265240 branch Apr 23, 2021

openjdk / jdk Public

8265240: runtime/Thread/SuspendAtExit.java needs updating #3576

8265240: runtime/Thread/SuspendAtExit.java needs updating #3576

dcubed-ojdk commented Apr 19, 2021 •

edited by openjdk[bot] bot

dcubed-ojdk commented Apr 19, 2021

bridgekeeper bot commented Apr 19, 2021

dcubed-ojdk commented Apr 19, 2021

openjdk bot commented Apr 19, 2021

openjdk bot commented Apr 19, 2021

robehn left a comment •

edited

openjdk bot commented Apr 20, 2021 •

edited

dcubed-ojdk commented Apr 20, 2021

mlbridge bot commented Apr 20, 2021 •

edited

dholmes-ora left a comment

dholmes-ora Apr 21, 2021

dcubed-ojdk Apr 21, 2021

dholmes-ora Apr 21, 2021

dcubed-ojdk Apr 21, 2021

dholmes-ora Apr 21, 2021

dcubed-ojdk Apr 21, 2021

dholmes-ora Apr 21, 2021

dcubed-ojdk Apr 21, 2021

dholmes-ora Apr 22, 2021

dholmes-ora Apr 22, 2021 •

edited

dcubed-ojdk Apr 22, 2021

dholmes-ora commented Apr 22, 2021

dcubed-ojdk commented Apr 23, 2021

openjdk bot commented Apr 23, 2021

		@@ -0,0 +1,73 @@
		/*
		* Copyright (c) 2001, 2021, Oracle and/or its affiliates. All rights reserved.

openjdk / jdk Public

8265240: runtime/Thread/SuspendAtExit.java needs updating #3576

8265240: runtime/Thread/SuspendAtExit.java needs updating #3576

Conversation

dcubed-ojdk commented Apr 19, 2021 • edited by openjdk[bot] bot

Progress

Issue

Reviewers

Reviewing

dcubed-ojdk commented Apr 19, 2021

bridgekeeper bot commented Apr 19, 2021

dcubed-ojdk commented Apr 19, 2021

openjdk bot commented Apr 19, 2021

openjdk bot commented Apr 19, 2021

robehn left a comment • edited

openjdk bot commented Apr 20, 2021 • edited

dcubed-ojdk commented Apr 20, 2021

mlbridge bot commented Apr 20, 2021 • edited

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora Apr 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora commented Apr 22, 2021

dcubed-ojdk commented Apr 23, 2021

openjdk bot commented Apr 23, 2021

dcubed-ojdk commented Apr 19, 2021 •

edited by openjdk[bot] bot

robehn left a comment •

edited

openjdk bot commented Apr 20, 2021 •

edited

mlbridge bot commented Apr 20, 2021 •

edited

dholmes-ora Apr 22, 2021 •

edited