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

8266056: runtime/stringtable/StringTableCleaningTest.java failed with "RuntimeException: Missing Callback in [10, 11]" #3815

Closed

Conversation

pliden
Copy link
Contributor

@pliden pliden commented Apr 30, 2021

This patch addresses two issues with the StringTableCleaningTest test.

  1. The test doesn't fully take into account that a concurrent GC might not complete (it can abort if the VM wants to terminate). findCallback() should return -1 if no callbacks are found, instead of failing the test.

  2. The regexp patterns used for ZGC needs to be more exact. The end pattern now also matches aborted GCs, which it shouldn't. This leads to "Garbage Collection (*) Aborted" being incorrectly matched as a GC end.

This patch also makes sure the output from the test VM ends up in the test log, otherwise it's impossible to tell what went wrong if this test fails in the future.

This patch also removes the test from the ProblemList.

Testing:

  • Passed Tier1-3.
  • Manual testing of StringTableCleaningTest with various GCs.

Progress

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

Issue

  • JDK-8266056: runtime/stringtable/StringTableCleaningTest.java failed with "RuntimeException: Missing Callback in [10, 11]"

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3815

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

Using diff file

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

… "RuntimeException: Missing Callback in [10, 11]"
@pliden
Copy link
Contributor Author

@pliden pliden commented Apr 30, 2021

/cc hotspot-gc

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 30, 2021

👋 Welcome back pliden! 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.

@pliden
Copy link
Contributor Author

@pliden pliden commented Apr 30, 2021

/cc hotspot-runtime

@openjdk openjdk bot added the hotspot-gc label Apr 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 30, 2021

@pliden
The hotspot-gc label was successfully added.

@openjdk openjdk bot added the hotspot-runtime label Apr 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 30, 2021

@pliden
The hotspot-runtime label was successfully added.

@pliden pliden marked this pull request as ready for review Apr 30, 2021
@openjdk openjdk bot added the rfr label Apr 30, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 30, 2021

Webrevs

Copy link
Contributor

@coleenp coleenp left a comment

Looks good and trivial.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 30, 2021

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

8266056: runtime/stringtable/StringTableCleaningTest.java failed with "RuntimeException: Missing Callback in [10, 11]"

Reviewed-by: coleenp, dholmes

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

  • f86b70c: 8266328: C2: Remove InlineWarmCalls
  • 928d632: 8252237: C2: Call to compute_separating_interferences has wrong argument order
  • 50fa162: 8266389: ProblemList java/awt/Graphics2D/DrawString/DrawRotatedStringUsingRotatedFont.java on generic-all
  • dd05158: 8266155: Convert java.base to use Stream.toList()
  • c36c63a: 8260560: convert jdeps and jdeprscan tools to use Stream.toList()
  • 096e9e5: 8266318: Switch to macos prefix for macOS bundles
  • 0544a73: 8255227: java/net/httpclient/FlowAdapterPublisherTest.java intermittently failing with TestServer: start exception: java.io.IOException: Invalid preface
  • 48bb996: 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified
  • 87de5b7: 8266040: Lanai: Incorrect calculations of clipping boundaries
  • eb8db12: 8263396: Atomic::CmpxchgByteUsingInt::set_byte_in_int needs an explicit cast
  • ... and 2 more: https://git.openjdk.java.net/jdk/compare/e9370a13b6f3f99d223ef5966f9e218b94d954b4...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 Apr 30, 2021
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Functional fix seems fine but a suggested change to the reporting.

Thanks,
David

output.outputTo(System.out);
output.errorTo(System.out);
Copy link
Member

@dholmes-ora dholmes-ora Apr 30, 2021

Choose a reason for hiding this comment

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

The normal way to do this is to call output.reportDiagnosticSummary(). But you only do that after the actual checks, when you know you have success, as in the error cases it will be called anyway and you don't want the output twice.

Copy link
Contributor Author

@pliden pliden Apr 30, 2021

Choose a reason for hiding this comment

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

Hmm, I see now that adding lines those is unnecessary, so I'll just remove them. Both shouldHaveExitValue() and checkOutput() seem to do the right thing. Not sure why I thought they didn't.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

I had assumed you wanted to see the output even on success. :)

Update is fine.

Thanks,
David

@pliden
Copy link
Contributor Author

@pliden pliden commented May 3, 2021

Thanks @dholmes-ora and @coleenp for reviewing!

/integrate

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

@openjdk openjdk bot commented May 3, 2021

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

  • 8fa50eb: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction
  • 7e30130: 8266401: mark hotspot compiler/intrinsics/sha/cli tests which ignore VM flags
  • dedddd5: 8266248: Compilation failure in PLATFORM_API_MacOSX_MidiUtils.c with Xcode 12.5
  • 5c083e8: 8266129: tools/jpackage/windows/WinInstallerIconTest.java hangs with fastdebug
  • 3e667cc: 8265356: need code example for getting canonical constructor of a Record
  • f86b70c: 8266328: C2: Remove InlineWarmCalls
  • 928d632: 8252237: C2: Call to compute_separating_interferences has wrong argument order
  • 50fa162: 8266389: ProblemList java/awt/Graphics2D/DrawString/DrawRotatedStringUsingRotatedFont.java on generic-all
  • dd05158: 8266155: Convert java.base to use Stream.toList()
  • c36c63a: 8260560: convert jdeps and jdeprscan tools to use Stream.toList()
  • ... and 7 more: https://git.openjdk.java.net/jdk/compare/e9370a13b6f3f99d223ef5966f9e218b94d954b4...master

Your commit was automatically rebased without conflicts.

Pushed as commit c5dc657.

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

@pliden pliden deleted the 8266056_string_table_cleaning_test branch May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc hotspot-runtime integrated
3 participants