On Mon, 8 Nov 2021 16:05:12 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this change which seeks to implement the >> enhancement noted in https://bugs.openjdk.java.net/browse/JDK-8153133? >> >> The commit in this PR uses the `StackWalker` API to dump the stacktrace of >> the thread. A few things to note about this change: >> >> - Previously, the dumped stacktrace would be preceded by a line (from the >> `Exception` instance) which would state `java.lang.Exception: Stack trace` >> and the rest of the lines would be the stacktrace. The change in this PR >> does a small (unrelated) enhacement which now includes the thread name in >> the message. So something like `MainThread Stack trace` where `MainThread` >> is the name of the thread whose stacktrace is being dumped. The linked JBS >> doesn't mention this change as a necessity but I decided to include it in >> this PR since I've always missed the thread name in that dumped stacktrace >> and since we are now using the StackWalker API, the mention of >> `java.lang.Exception: ...` line in the thread dump will no longer make sense >> (unless we wanted backward compatibility) >> - The change doesn't use lambda to allow `Thread.dumpStack()` to be called >> (for debugging purposes) from places where lambda usage might be too early. >> - The change also includes a fallback to use the >> `Exception.printStackTrace()` to allow for calls to `Thread.dumpStack()` >> when `StackWalker` class itself is being initialized. >> >> The PR also includes a new jtreg test which verifies this change. The test >> has 3 test actions - one without security manager, one with security manager >> and one with `java.security.debug=access,stack` to exercise the case where >> Thread.dumpStack() gets called during StackWalker class initialization. >> >> The linked JBS issue notes that it would be good to use the StackWalker API >> even in the `Thread.getStackTrace` implementation. This PR however doesn't >> include that change because I wanted to tackle that separately since I think >> that would need to run some performance benchmarks to evaluate any >> performance changes. The current `Thread.dumpStack` method clearly states >> that it is meant for debugging purposes so I haven't run any kind of >> performance benchmarks on this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix jcheck failure - convert tab to space @jaikiran Thanks for making this change and identifying the recursive initialization issue. I also concur with Alan that replacing Thread::dumpStack implementation with StackWalker does not buy us much. It has to fallback to `new Throwable().printStackTrace()` in some cases. I think we should close this RFE as will not fix. ------------- PR: https://git.openjdk.java.net/jdk/pull/6292
