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

8272348: Update CDS tests in anticipation of JDK-8270489 #5095

Conversation

iklam
Copy link
Member

@iklam iklam commented Aug 11, 2021

For testing #5074 (Support archived heap objects in EpsilonGC), some CDS tests should be updated:

(1) Add @require vm.gc == null to mark test cases that will fail if a collector is explicitly specified using -XX:+UseXXXGC

(2) Rename this WhiteBox API to reflect what it checks:

!WB.areSharedStringsIgnored() -> WB.areSharedStringsMapped()

With EpsilonGC, the shared strings are not "ignored", but they are also not "mapped" either (they are copied into the Java heap). The existing tests are for mapped strings only, so they should check for WB.areSharedStringsMapped().

(3) For test cases that check if a heap object is a shared interned string, instead of the generic WB.isShared(Object o) API, use a new specific API WB.isSharedInternedString(String s). This API currently works only for "mapped shared strings", but in the future we may enhance it to check for "loaded shared strings" by EpsilonGC.

(4) Also fix the two test cases that were sensitive to irregular -Xlog output:

  • ServiceLoaderTest.java: remove timestamp decorations from the logs
  • DumpClassList.java: handle extra space characters that may appear in the tag decoration between square brackets.

Progress

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

Issue

  • JDK-8272348: Update CDS tests in anticipation of JDK-8270489

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5095

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 11, 2021

👋 Welcome back iklam! 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
Copy link

@openjdk openjdk bot commented Aug 11, 2021

@iklam The following label will be automatically applied to this pull request:

  • hotspot

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

@openjdk openjdk bot added the hotspot label Aug 11, 2021
@iklam
Copy link
Member Author

@iklam iklam commented Aug 11, 2021

/label remove hotspot
/label add hotspot-runtime

@openjdk openjdk bot removed the hotspot label Aug 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 11, 2021

@iklam
The hotspot label was successfully removed.

@openjdk openjdk bot added the hotspot-runtime label Aug 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 11, 2021

@iklam
The hotspot-runtime label was successfully added.

@iklam iklam marked this pull request as ready for review Aug 12, 2021
@openjdk openjdk bot added the rfr label Aug 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 12, 2021

Webrevs

public native boolean areSharedStringsIgnored();
public native boolean areSharedStringsMapped();
public native boolean isSharedInternedString(String s);
Copy link
Member

@iignatev iignatev Aug 12, 2021

Choose a reason for hiding this comment

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

please also update jdk.test.whitebox.WhiteBox

Copy link
Member Author

@iklam iklam Aug 13, 2021

Choose a reason for hiding this comment

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

I have synced jdk/test/whitebox/WhiteBox.java to match the latest APIs in sun/hotspot/WhiteBox.java

…h the latest sun/hotspot/WhiteBox.java APIs
Copy link
Member

@calvinccheung calvinccheung left a comment

LGTM.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 13, 2021

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

8272348: Update CDS tests in anticipation of JDK-8270489

Reviewed-by: ccheung, minqi

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

  • 87d2761: 8271883: Math CopySign optimization for x86
  • 6b8b160: 8272396: mismatching debug output streams
  • 0af645a: 8205138: Remove Applet references from Font2DTest
  • bd7f9b4: 8272459: ProblemList compiler/codecache/TestStressCodeBuffers.java on aarch64
  • 717792c: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path
  • d06d0b9: 8272112: Arena code simplifications
  • 0c4be76: 8058704: Nimbus does not honor JTextPane background color
  • 020aec5: 8271366: [REDO] JDK-8266054 VectorAPI rotate operation optimization
  • 4d4ba5c: 8272116: Update PerfDisableSharedMem with FLAG_SET_ERGO in PerfMemory::create_memory_region
  • 09ab86b: 8269909: getStack method in hprof.parser.Reader should use try-with-resource
  • ... and 12 more: https://git.openjdk.java.net/jdk/compare/cd2dbe5f007baf81ae9262c1152917e620970621...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.

@openjdk openjdk bot added the ready label Aug 13, 2021
Copy link
Contributor

@yminqi yminqi left a comment

LG, one question of the added function handshakeReadMonitor

@@ -605,6 +606,7 @@ public Object getMethodOption(Executable method, String name) {

// Handshakes
public native int handshakeWalkStack(Thread t, boolean all_threads);
public native boolean handshakeReadMonitors(Thread t);
Copy link
Contributor

@yminqi yminqi Aug 13, 2021

Choose a reason for hiding this comment

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

Why is this function added?

Copy link
Member Author

@iklam iklam Aug 14, 2021

Choose a reason for hiding this comment

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

We have two WhiteBox classes: sun.hotspot.WhiteBox and jdk.test.whitebox.WhiteBox. They should have the exact same contents, but handshakeReadMonitors is missing from jdk.test.whitebox.WhiteBox. So I added it since I am changing this file.

https://bugs.openjdk.java.net/browse/JDK-8271707 will remove sun.hotspot.WhiteBox so we won't have this duplication anymore.

yminqi
yminqi approved these changes Aug 14, 2021
Copy link
Contributor

@yminqi yminqi left a comment

Then it is Good to go!

@iklam
Copy link
Member Author

@iklam iklam commented Aug 16, 2021

Thanks @iignatev @calvinccheung and @yminqi for your review.
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Aug 16, 2021

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

  • 3f38a50: 8271203: C2: assert(iff->Opcode() == Op_If || iff->Opcode() == Op_CountedLoopEnd || iff->Opcode() == Op_RangeCheck) failed: Check this code when new subtype is added
  • 6a5241c: 8272491: Problem list javax/swing/JFrame/NSTexturedJFrame/NSTexturedJFrame.java on macos
  • 17b9350: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL
  • 87d2761: 8271883: Math CopySign optimization for x86
  • 6b8b160: 8272396: mismatching debug output streams
  • 0af645a: 8205138: Remove Applet references from Font2DTest
  • bd7f9b4: 8272459: ProblemList compiler/codecache/TestStressCodeBuffers.java on aarch64
  • 717792c: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path
  • d06d0b9: 8272112: Arena code simplifications
  • 0c4be76: 8058704: Nimbus does not honor JTextPane background color
  • ... and 15 more: https://git.openjdk.java.net/jdk/compare/cd2dbe5f007baf81ae9262c1152917e620970621...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Aug 16, 2021

@iklam Pushed as commit 36e2dda.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated
5 participants