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

8263460: DynamicArchiveRelocationTest.java fails in product VM #2947

Conversation

iklam
Copy link
Member

@iklam iklam commented Mar 11, 2021

Please review this test bug.

The handling of -XX:ArchiveRelocationMode=1 is different between fastdebug and product builds:

  • fastdebug: (to improve test coverage) -- we always map at the default location, then unmap, and then map again at a random location.
  • product: (for faster performance) -- don't attempt to map at the default location. Just map at random location

Therefore, the "unmap" messages are not available in product mode.

I added comments to explain what output is expected, and rearranged the regexp for better matching.


Progress

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

Issue

  • JDK-8263460: DynamicArchiveRelocationTest.java fails in product VM

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2947/head:pull/2947
$ git checkout pull/2947

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 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 bot commented Mar 11, 2021

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

  • hotspot-runtime

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-runtime hotspot-runtime-dev@openjdk.org label Mar 11, 2021
@iklam iklam marked this pull request as ready for review March 11, 2021 19:08
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 11, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 11, 2021

Webrevs

Copy link
Member

@calvinccheung calvinccheung left a comment

Choose a reason for hiding this comment

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

LGTM.

@openjdk
Copy link

openjdk bot commented Mar 11, 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:

8263460: DynamicArchiveRelocationTest.java fails in product VM

Reviewed-by: ccheung, dcubed

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

  • b932a62: 8263470: Consolidate copies of getClassBytes in various tests
  • 0ea48d9: 8194129: Regression automated Test '/open/test/jdk/java/awt/Window/ShapedAndTranslucentWindows/TranslucentChoice.java' fails
  • 4b5c664: 8178348: left_n_bits(0) invokes undefined behavior
  • 0b10c6b: 8263017: Read barriers are missing in nmethod printing code
  • a6e056f: 8263125: During deoptimization vectors should reassign scalarized payload after all objects are reallocated.
  • 65421fa: 8213177: GlobalCounter::CSContext could be an enum class
  • a9b156d: 8258414: OldObjectSample events too expensive
  • 0bbe064: 8263354: Accumulated C2 code cleanups
  • aa33443: 8262454: Handshake timeout improvements, single target, kill unfinished thread
  • ff25939: 8263426: Reflow JfrNetworkUtilization::send_events
  • ... and 7 more: https://git.openjdk.java.net/jdk/compare/7ed46bd02e66415f3a9ce03b93e4469f9277aff5...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 Pull request is ready to be integrated label Mar 11, 2021
Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

The special handling for Windows is not needed anymore?

I have the feeling this could be made simpler in general (not in this fix). E.g. instead of mapping, then unmapping, which causes the execution paths between non-product and product to be quite different, why not just reserve a "rogue" allocation in the middle of the soon-to-map region? Would that not be a more reliable and realistic test?

Cheers, Thomas

@dcubed-ojdk
Copy link
Member

@ioilam - Any chance that this fix will get integrated before the weekend?
I'm interested in taking the two failures per Tier6 off the board for the weekend.

@iklam
Copy link
Member Author

iklam commented Mar 12, 2021

The special handling for Windows is not needed anymore?

Hi Thomas, thanks for looking into this.

You're correct that the Windows port may work differently. When mapping to the requested address, some of the regions may fail to map due to ASLR, so you are not guaranteed to have two "Unmapping region ..." messages.

Also, this is theoretically not limited to Windows, either. Some other OS ports may also make the requested address unavailable due to ASLR.

Therefore, I decided to get rid of the pattern matching altogether. We know that the native code will try its best to exercise the mapping and unmapping code, so as long as the program exits normally, we know all has gone well :-)

I have the feeling this could be made simpler in general (not in this fix). E.g. instead of mapping, then unmapping, which causes the execution paths between non-product and product to be quite different, why not just reserve a "rogue" allocation in the middle of the soon-to-map region? Would that not be a more reliable and realistic test?

Cheers, Thomas

Such a "rouge" region would need to be allocated inside the VM code, since we can't easily do it from the "driver" process of the test, Also, this will cause at least one of the regions to fail to map. As a result, we cannot test the case where all the regions are mapped, but then we have to unmap them due to other reasons.

Admittedly, a different way to test this is to corrupt the CRC of one of the regions. This should be done in appcds/SharedArchiveConsistency.java but we exit the VM and don't exercise the code for unmapping. I have created JDK-8263538 (SharedArchiveConsistency.java should test -Xshare:auto as well)

@iklam
Copy link
Member Author

iklam commented Mar 12, 2021

@ioilam - Any chance that this fix will get integrated before the weekend?
I'm interested in taking the two failures per Tier6 off the board for the weekend.

Yes, if I can get enough reviews for my latest version :-)

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

It looks to me like you've done this:

Therefore, I decided to get rid of the pattern matching altogether

so thumbs up!
I presume you've tested the latest version with 'release' bits.

@iklam
Copy link
Member Author

iklam commented Mar 12, 2021

It looks to me like you've done this:

Therefore, I decided to get rid of the pattern matching altogether

so thumbs up!
I presume you've tested the latest version with 'release' bits.

Yes, I tested the latest version with both debug and release bits.

@iklam
Copy link
Member Author

iklam commented Mar 12, 2021

/integrate

@openjdk openjdk bot closed this Mar 12, 2021
@openjdk 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 Mar 12, 2021
@openjdk
Copy link

openjdk bot commented Mar 12, 2021

@iklam Since your change was applied there have been 19 commits pushed to the master branch:

  • b2f7c58: 8263055: hsdb Command Line Debugger does not properly direct output for some commands
  • ecfa712: 8263326: Remove ReceiverTypeData check from serviceability/sa/TestPrintMdo.java
  • b932a62: 8263470: Consolidate copies of getClassBytes in various tests
  • 0ea48d9: 8194129: Regression automated Test '/open/test/jdk/java/awt/Window/ShapedAndTranslucentWindows/TranslucentChoice.java' fails
  • 4b5c664: 8178348: left_n_bits(0) invokes undefined behavior
  • 0b10c6b: 8263017: Read barriers are missing in nmethod printing code
  • a6e056f: 8263125: During deoptimization vectors should reassign scalarized payload after all objects are reallocated.
  • 65421fa: 8213177: GlobalCounter::CSContext could be an enum class
  • a9b156d: 8258414: OldObjectSample events too expensive
  • 0bbe064: 8263354: Accumulated C2 code cleanups
  • ... and 9 more: https://git.openjdk.java.net/jdk/compare/7ed46bd02e66415f3a9ce03b93e4469f9277aff5...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0c8350e.

💡 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 hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
5 participants