8320707: Virtual thread test updates

[AlanBateman]

A lot of test changes have accumulated in the loom repo, this includes both new tests and updates to existing tests. Some of these updates can be brought to the main line. This update brings over:

• The existing tests for pinning use synchronized blocks. In preparation for changes to allow carrier thread be released when a virtual thread parks holding a monitor or blocks on monitorenter, these tests are changed to pin by having a native frame on the stack. This part includes test infrastructure to make it easy to add more tests that do operations while pinned. The tests still test what they were originally created to test of course.

• The test for the JFR jdk.VirtualThreadPinned event is refactored to allow for additional cases where the event may be reported.

• ThreadAPI is expanded to cover test for uncaught exception handling.

• GetStackTraceWhenRunnable is refactored to not use a Selector, otherwise this test will be invalidated when blocking selection operations release the carrier.

• StressStackOverflow is dialed down to run for 1m instead of 2mins.

• The use of CountDownLatch in a number of tests that poll thread state has been dropped to keep the tests as simple as possible.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8320707: Virtual thread test updates (Enhancement - P4)

Reviewers

• Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17136/head:pull/17136
$ git checkout pull/17136

Update a local copy of the PR:
$ git checkout pull/17136
$ git pull https://git.openjdk.org/jdk.git pull/17136/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17136

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17136.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back alanb! 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]

@AlanBateman The following labels will be automatically applied to this pull request:

• core-libs
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (fe255921)
• 00: Full (b7abfc0a)

[plummercj]

Can you explain your motivation for using AtomicBoolean with a polling loop rather than CountDownLatch(1)? I'm working on a test where I just added a CountDownLatch(1) and am wondering if I should do the same, but I'm not sure if there is something about these tests that is motivating the change.

[AlanBateman]

I'm working on a test where I just added a CountDownLatch(1) and am wondering if I should do the same, but I'm not sure if there is something about these tests that is motivating the change.

CountDownLatch is great for many tests. It's not as powerful as a Phaser of course but good enough and usually easy to understand quickly what is going on. However, for tests that are testing thread state then you often want to have as few dependencies as possible. In the case of CountDownlatch, the await method may park. The countDown method may have to unpark waiters for for virtual threads it means potentially parking (to queue a task) when unparking, so temporary transitions that JVMTI has to be concerned with. The other thing is keeping tests simple/consistent, it can be hard to maintain tests where one test method coordinates with one approach, another test method does something different.

[jaikiran]

Overall these changes look OK to me. I haven't paid much attention to the jvmti test change, but I've looked at others and they seem fine.
The StressStackOverflow test has its duration reduced by half which appears intentional and then there's a new test library helper class to allow pinning tasks in tests.

Just one question - doesn't the use of a new native code in the test library (the libVThreadPinner.c) require any build changes (I'm not too familiar with that part)?

[openjdk]

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

8320707: Virtual thread test updates

Reviewed-by: jpai

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

• a8e4229: 8322783: prioritize /etc/os-release over /etc/SuSE-release in hs_err/info output
• cbe329b: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder
• 06dd735: 8322766: Micro bench SSLHandshake should use default algorithms
• 9ab29f8: 8321718: ProcessTools.executeProcess calls waitFor before logging
• ba426d6: 8322841: Parallel: Remove unused using-declaration in MutableNUMASpace
• 18cdc90: 8322801: RISC-V: The riscv path of the debian sysroot had been changed
• fcf8368: 8322248: Fix inconsistent wording in ElementFilter.typesIn
• a678416: 8322805: Eliminate -Wparentheses warnings in x86 code
• 122bc77: 8322758: Eliminate -Wparentheses warnings in C2 code
• e9e694f: 8320275: assert(_chunk->bitmap().at(index)) failed: Bit not set at index
• ... and 15 more: https://git.openjdk.org/jdk/compare/32d80e2caf6063b58128bd5f3dc87b276f3bd0cb...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.

[AlanBateman]

Just one question - doesn't the use of a new native code in the test library (the libVThreadPinner.c) require any build changes (I'm not too familiar with that part)?

Magnus did some work in the make files some time ago to build native libs in the test/lib tree.

Thanks for reviewing.

[AlanBateman]

/integrate

[openjdk]

Going to push as commit b67b71c.
Since your change was applied there have been 27 commits pushed to the master branch:

• 7eb25ec: 8322853: Should use ConditionalMutexLocker in NativeHeapTrimmerThread::print_state
• 539da24: 8322779: C1: Remove the unused counter 'totalInstructionNodes'
• a8e4229: 8322783: prioritize /etc/os-release over /etc/SuSE-release in hs_err/info output
• cbe329b: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder
• 06dd735: 8322766: Micro bench SSLHandshake should use default algorithms
• 9ab29f8: 8321718: ProcessTools.executeProcess calls waitFor before logging
• ba426d6: 8322841: Parallel: Remove unused using-declaration in MutableNUMASpace
• 18cdc90: 8322801: RISC-V: The riscv path of the debian sysroot had been changed
• fcf8368: 8322248: Fix inconsistent wording in ElementFilter.typesIn
• a678416: 8322805: Eliminate -Wparentheses warnings in x86 code
• ... and 17 more: https://git.openjdk.org/jdk/compare/32d80e2caf6063b58128bd5f3dc87b276f3bd0cb...master

Your commit was automatically rebased without conflicts.

[openjdk]

@AlanBateman Pushed as commit b67b71c.

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