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

JDK-8261520: JDK-8261302 breaks runtime/NMT/CheckForProperDetailStackTrace.java #2672

Conversation

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Feb 22, 2021

Since JDK-8261302, the test runtime/NMT/CheckForProperDetailStackTrace.java fails with

java.lang.RuntimeException: 'NativeCallStack::NativeCallStack' found in stdout

--

NativeCallStack contains a hash code. Before JDK-8261302, that hash code was calculated lazily in a non-inline hashcode getter. With JDK-8261302, the hash code calculation was moved into the NativeCallStack constructor and the getter was made inline.

The NativeCallStack constructor fills itself via os::get_native_stack(). Before JDK-8261302, that call has been the last call in the constructor and hence had been sometimes optimized into a tail call. Whether or not its a tail call matters since it affects the number of stack frames the stack walker has to skip. Therefore, the constructor contains coding to predict tail-call-ness:

#if (defined(_NMT_NOINLINE_) || defined(_WINDOWS) || !defined(_LP64) || defined(PPC64))
  // Not a tail call.
  toSkip++;
#if (defined(_NMT_NOINLINE_) && defined(BSD) && defined(_LP64))
  // Mac OS X slowdebug builds have this odd behavior where NativeCallStack::NativeCallStack
  // appears as two frames, so we need to skip an extra frame.
  toSkip++;
#endif // Special-case for BSD.
#endif // Not a tail call.

This prediction was now off since the hash code calculation happened at the end of the callstack. This causes the test error, since on some platforms (eg Linux x64) we now think we have a tail call when we don't, which means we do not skip enough frames, and the NMT output contains call frames like "NativeCallStack::NativeCallStack()", which trips the test.


Fix:

This fix moves the hash code calculation completely out of NativeCallStack. There is no reason why NativeCallStack should have a hash code. It mainly exists as a convenience to place it in a hash map. The patch moves the hash code calculation up into MallocSiteTableEntry.

This has the advantage of only having to pay for a hash code when you need it - in theory, one may use NativeCallStack in places other than NMT, where it is unnecessary.

I considered other options:

  • modify os::get_native_stack() to also calculate a hash in addition to capturing the stack, and return it in a caller provided variable. That would have left this call to be the tail call. However, it seemed less clean - we have two implementations of this function, as well as other, non-capturing, NativeCallStack constructors, which would have to be modified. It also would have made os::get_native_stack() less general purpose.
  • Leave it as it is and just always skip frames: Seemed attractive, but I did not want to touch the tailcode-prediction-code and play whack-the-mole with platform specific test errors.

Tests: GA, manual test, nightlies at SAP


Progress

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

Issue

  • JDK-8261520: JDK-8261302 breaks runtime/NMT/CheckForProperDetailStackTrace.java

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 22, 2021

👋 Welcome back stuefe! 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 Feb 22, 2021

@tstuefe 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 Feb 22, 2021
@tstuefe tstuefe changed the title Initial JDK-8261520: JDK-8261302 breaks runtime/NMT/CheckForProperDetailStackTrace.java Feb 22, 2021
@tstuefe tstuefe marked this pull request as ready for review Feb 23, 2021
@openjdk openjdk bot added the rfr label Feb 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 23, 2021

Webrevs

@@ -57,12 +57,13 @@ class MallocSite : public AllocationSite {
class MallocSiteHashtableEntry : public CHeapObj<mtNMT> {
private:
MallocSite _malloc_site;
const unsigned _hash;
Copy link
Contributor

@zhengyu123 zhengyu123 Feb 24, 2021

Prefer "unsigned int" to be consist with other places. Otherwise, Looks good to me.

Copy link
Member Author

@tstuefe tstuefe Feb 24, 2021

Thank you Zhengyu!

I reverted to unsigned int. Using just unsigned is a bad habit of mine, sorry.

Cheers, Thomas

@openjdk
Copy link

@openjdk openjdk bot commented Feb 24, 2021

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

8261520: JDK-8261302 breaks runtime/NMT/CheckForProperDetailStackTrace.java

Reviewed-by: zgu, coleenp

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

  • 059ede0: 8262428: doclint warnings in java.xml module
  • 8256517: 8262421: doclint warnings in jdk.compiler module
  • 29c603f: 8262227: Change SystemDictionary::find() to return an InstanceKlass*.
  • 35c0a69: 8262416: ProblemList TestHeapDumpForLargeArray.java due to JDK-8262386
  • 228c285: 8261170: Upgrade to freetype 2.10.4
  • ded96dd: 8139348: Deprecate 3DES and RC4 in Kerberos
  • 5a9b701: 8258444: Clean up specifications of java.io.Reader.read(char[],int,int) in subclass overrides
  • 7d4f60b: 8260403: javap should be more robust in the face of invalid class files
  • 674be87: 8261203: Incorrectly escaped javadoc html with type annotations
  • 2eca17d: 8261457: test/langtools/tools/javac/T8187978 can fail if ArrayList class is modified
  • ... and 8 more: https://git.openjdk.java.net/jdk/compare/a50725db2ab621e1a17cb5505f78e4bc73972a27...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 Feb 24, 2021
Copy link
Contributor

@coleenp coleenp left a comment

Yes this makes sense. I use NativeCallStack sometimes for debugging things and don't need the hash code, so this is good.

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 26, 2021

Yes this makes sense. I use NativeCallStack sometimes for debugging things and don't need the hash code, so this is good.

Yes, I do too. Also, I had some vague thoughts once about using it in UL to provide callstacks at log sites (was actually Robins idea).

Thanks!

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 26, 2021

/integrate

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

@openjdk openjdk bot commented Feb 26, 2021

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

  • bcca100: 4710675: JTextArea.setComponentOrientation does not work with correct timing
  • fce5765: 8262433: doclint: reference error in module jdk.incubator.foreign
  • 059ede0: 8262428: doclint warnings in java.xml module
  • 8256517: 8262421: doclint warnings in jdk.compiler module
  • 29c603f: 8262227: Change SystemDictionary::find() to return an InstanceKlass*.
  • 35c0a69: 8262416: ProblemList TestHeapDumpForLargeArray.java due to JDK-8262386
  • 228c285: 8261170: Upgrade to freetype 2.10.4
  • ded96dd: 8139348: Deprecate 3DES and RC4 in Kerberos
  • 5a9b701: 8258444: Clean up specifications of java.io.Reader.read(char[],int,int) in subclass overrides
  • 7d4f60b: 8260403: javap should be more robust in the face of invalid class files
  • ... and 10 more: https://git.openjdk.java.net/jdk/compare/a50725db2ab621e1a17cb5505f78e4bc73972a27...master

Your commit was automatically rebased without conflicts.

Pushed as commit 722142e.

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

@tstuefe tstuefe deleted the JDK-8261520-fix-runtime-NMT-CheckForProperDetailStackTrace-java branch Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants