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.

Sign up for GitHub

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

Jump to bottom

8253877: gc/g1/TestGCLogMessages.java fails - missing "Evacuation failure" message #624

Closed
wants to merge 1 commit into from
Closed

8253877: gc/g1/TestGCLogMessages.java fails - missing "Evacuation failure" message #624

wants to merge 1 commit into from

Conversation

tschatzl
Copy link
Contributor

@tschatzl tschatzl commented Oct 13, 2020 •
edited by openjdk bot
Loading

Hi all,

can I have reviews for this change to gc/g1/TestGCLogMessages to hopefully make it more reliable?

One subtest tries to force evacuation failure, but apparently this does not always succeed. Particularly with upcoming young gen sizing changes, there seems to be a larger than before chance that the current mechanism does not work.

This mechanism assumes that before that test the young gen has been sized that much that the allocation of a half-heap humongous object causes an evacuation failure next time - of course that's a somewhat dodgy assumption that apparently recently started failing more.

The change employs the G1 internal evacuation failure debugging mechanism to force this kind of situation. The advantage is that this is guaranteed to work, the disadvantage is that it relies on that feature.

Additionally I did some slight renaming to the helper that used the term "ToSpaceExhaustion" instead of "EvacuationFailure" which is the name of the message that is actually checked for.

Testing: 2k+ tests of previous version without reproduction (meaning that at least for me without context of other tests, the issue has not been reproducable, at least not with the mentioned 1% failure rate), 3k tests with new version without reproduction

Thanks,
Thomas

Progress

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

Issue

  • JDK-8253877: gc/g1/TestGCLogMessages.java fails - missing "Evacuation failure" message

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 13, 2020

👋 Welcome back tschatzl! 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 added the rfr Pull request is ready for review label Oct 13, 2020
@openjdk
Copy link

openjdk bot commented Oct 13, 2020

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

  • hotspot-gc

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-gc hotspot-gc-dev@openjdk.org label Oct 13, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 13, 2020

Webrevs

@tschatzl
Copy link
Contributor Author

In the future there may be a better way to force "Evacuation Failure" with e.g. forcibly pinning regions in young gen - but then again this would not be called an evacuation failure.

kstefanj
kstefanj approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@kstefanj kstefanj left a comment

Choose a reason for hiding this comment

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

I think it is good to make use of the debug feature to ensure stable testing and I'm approving this change.

I guess an approach if we don't want to use it is to just have an allocation loop that keeps everything live, it should trigger evacuation failures before throwing OOME.

@openjdk
Copy link

openjdk bot commented Oct 13, 2020 •
edited
Loading

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

8253877: gc/g1/TestGCLogMessages.java fails - missing "Evacuation failure" message

Reviewed-by: sjohanss

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

  • 1742c44: 8254695: G1: Next mark bitmap clear not cancelled after marking abort
  • 34583eb: 8254161: Prevent instantiation of EnumSet subclasses through deserialization
  • 3d23bd8: 8254814: [Vector API] Fix an AVX512 crash after JDK-8223347
  • 7c0d417: 8251535: Partial peeling at unsigned test adds incorrect loop exit check
  • 5145bed: 8254125: Assertion in cppVtables.cpp during builds on 32bit Windows
  • bdda205: 8254369: Node::disconnect_inputs may skip precedences
  • 96bb6e7: 8251325: Miss 'L' for long value in if statement
  • 546620b: 8254192: ExtraSharedClassListFile contains extra white space at end of line
  • f3ce45f: 8254799: runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java fails with release VMs
  • cda22e3: 8254811: JDK-8254158 broke ppc64, s390 builds
  • ... and 48 more: https://git.openjdk.java.net/jdk/compare/90de2894e90c0808ec98857bcd562897d14e2ffb...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 Oct 13, 2020
@tschatzl
Copy link
Contributor Author

Exactly the catch-OOME approach seemed too error-prone. We may not recover from the OOME handler.

@kstefanj
Copy link
Contributor

Exactly the catch-OOME approach seemed too error-prone. We may not recover from the OOME handler.

I agree, trying to recover from OOME is very error-prone. In this case since the test is starting in its own process it might be fine to just let it exit with an OOME. We would have to change the check for exit-code 0, but I think it should work.

Not saying this is a better approach, just threw it out here :) Again, I'm good with the change you propose.

@tschatzl
Copy link
Contributor Author

/integrate

@openjdk openjdk bot closed this Oct 19, 2020
@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 Oct 19, 2020
@openjdk
Copy link

openjdk bot commented Oct 19, 2020

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

Your commit was automatically rebased without conflicts.

Pushed as commit cd66e0f.

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

@tschatzl tschatzl deleted the 8253877-testgclogmessages-improv branch October 20, 2020 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kstefanj kstefanj kstefanj approved these changes

Assignees
No one assigned
Labels
hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tschatzl @kstefanj