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

8320707: Virtual thread test updates #328

Closed

Conversation

shipilev
Copy link
Member

@shipilev shipilev commented Mar 5, 2024

Test-only backport that simplifies Loom maintenance. The original commit applies cleanly, but it does not work out of the box, because VThreadPinner uses FFM, which is a preview feature in JDK 21, made final in JDK 22. My attempts to soft-touch FFM into working in JDK 21 with these tests failed: x86_32's fallback linker does not work properly in JDK 21, so tests cannot complete there.

Therefore, I made a side-ways move here: rewrote VThreadPinner to use synchronized, which still pins the thread in JDK 21, and would likely continue doing so. It is unlikely we would backport the runtime changes required to avoid pinning on synchronized blocks. This is what the old tests did anyway.

This would also make subsequent backports clean.

Additional testing:

  • MacOS AArch64 server fastdebug, jdk_loom hotspot_loom

Progress

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

Issue

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

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk21u-dev.git pull/328/head:pull/328
$ git checkout pull/328

Update a local copy of the PR:
$ git checkout pull/328
$ git pull https://git.openjdk.org/jdk21u-dev.git pull/328/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 328

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk21u-dev/pull/328.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 5, 2024

👋 Welcome back shade! 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 openjdk bot changed the title Backport b67b71cd87c62f15d5b73f923c300d0f77c988f5 8320707: Virtual thread test updates Mar 5, 2024
@openjdk
Copy link

openjdk bot commented Mar 5, 2024

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Mar 5, 2024
@mlbridge
Copy link

mlbridge bot commented Mar 5, 2024

Webrevs

@shipilev
Copy link
Member Author

shipilev commented Mar 5, 2024

GHA x86_32 tests are failing, I think we need #330 first.

@shipilev
Copy link
Member Author

shipilev commented Mar 6, 2024

Grr, the x86_32 thing does not work. Getting back to draft until I can figure it out.

@shipilev shipilev marked this pull request as draft March 6, 2024 09:56
@openjdk openjdk bot removed the rfr Pull request is ready for review label Mar 6, 2024
@openjdk
Copy link

openjdk bot commented Mar 13, 2024

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

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

  • 031acf1: 8327096: (fc) java/nio/channels/FileChannel/Size.java fails on partition incapable of creating large files
  • 324cef1: 8327971: Multiple ASAN errors reported for metaspace
  • ac402fb: 8293850: need a largest_committed metric for each category of NMT's output
  • febbf35: 8316242: Opensource SwingGraphics manual test
  • 1861655: 8316154: Opensource JTextArea manual tests
  • a36d3de: 8328957: Update PKCS11Test.java to not use hardcoded path
  • bb046b7: 8320525: G1: G1UpdateRemSetTrackingBeforeRebuild::distribute_marked_bytes accesses partially unloaded klass
  • ffdde9c: 8320331: G1 Full GC Heap verification relies on metadata not reset before verification
  • d11a065: 8187759: Background not refreshed when painting over a transparent JFrame
  • 5880551: 8318854: [macos14] Running any AWT app prints Secure coding warning
  • ... and 19 more: https://git.openjdk.org/jdk21u-dev/compare/7743b6ca05b8486eeb38cbbe74569401ce62fe2a...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.

@shipilev shipilev force-pushed the JDK-8320707-loom-test-updates branch from fc6dd1f to 05d454d Compare April 18, 2024 13:18
@shipilev shipilev force-pushed the JDK-8320707-loom-test-updates branch from 05d454d to 1775292 Compare April 18, 2024 16:11
@shipilev shipilev marked this pull request as ready for review April 18, 2024 17:18
@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 18, 2024
@shipilev
Copy link
Member Author

@jerboaa, @GoeLin -- good for 21u, or?

@jerboaa
Copy link
Contributor

jerboaa commented Apr 22, 2024

Seems fine to me.

@jerboaa
Copy link
Contributor

jerboaa commented Apr 22, 2024

Being a good citizen would need a 22u backport first.

@shipilev
Copy link
Member Author

shipilev commented Apr 22, 2024

Seems fine to me.

Thanks!

Being a good citizen would need a 22u backport first.

Not sure we want to do tests maintenance in a soon-to-be-dead release, but here it is: openjdk/jdk22u#160

@shipilev
Copy link
Member Author

/approval request Test-only backport that simplifies Loom maintenance. The original commit applies cleanly, but does not work, because tests use FFM that are not stable in JDK 21, rewritten back to synchronized. See 21u PR for more details. Loom tests pass. Low risk, test-only change.

@openjdk
Copy link

openjdk bot commented Apr 22, 2024

@shipilev
8320707: The approval request has been created successfully.

@openjdk openjdk bot added the approval label Apr 22, 2024
Copy link
Member

@simonis simonis left a comment

Choose a reason for hiding this comment

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

Looks good!

Nice idea to use synchronized (runner).

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed approval labels Apr 25, 2024
@shipilev
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Apr 25, 2024

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

  • 955fcc1: 8310355: Move the stub test from initialize_final_stubs() to test/hotspot/gtest
  • 031acf1: 8327096: (fc) java/nio/channels/FileChannel/Size.java fails on partition incapable of creating large files
  • 324cef1: 8327971: Multiple ASAN errors reported for metaspace
  • ac402fb: 8293850: need a largest_committed metric for each category of NMT's output
  • febbf35: 8316242: Opensource SwingGraphics manual test
  • 1861655: 8316154: Opensource JTextArea manual tests
  • a36d3de: 8328957: Update PKCS11Test.java to not use hardcoded path
  • bb046b7: 8320525: G1: G1UpdateRemSetTrackingBeforeRebuild::distribute_marked_bytes accesses partially unloaded klass
  • ffdde9c: 8320331: G1 Full GC Heap verification relies on metadata not reset before verification
  • d11a065: 8187759: Background not refreshed when painting over a transparent JFrame
  • ... and 20 more: https://git.openjdk.org/jdk21u-dev/compare/7743b6ca05b8486eeb38cbbe74569401ce62fe2a...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 25, 2024
@openjdk openjdk bot closed this Apr 25, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 25, 2024
@openjdk
Copy link

openjdk bot commented Apr 25, 2024

@shipilev Pushed as commit 41237fc.

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

@shipilev shipilev deleted the JDK-8320707-loom-test-updates branch April 26, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

3 participants