Code Review for JEP 259: Stack-Walking API (original) (raw)

Mandy Chung mandy.chung at oracle.com
Wed Nov 18 22:24:02 UTC 2015

On Nov 18, 2015, at 11:00 AM, Brent Christian <brent.christian at oracle.com> wrote:

Hi, Mandy I looked through the jdk code. I think it's looking quite good. I just noticed some small details to consider fixing up before pushing. Docs: StackWalker.StackFrame.getClassName(): the "binary name" link seems to be broken (StackWalker.java line 100)

It works for me. It looks correct to me.

StackWalker.getInstance(Set<StackWalker.Option> options): StackWalker.getInstance(Set<StackWalker.Option> options, int estimateDepth):

A couple missing words: "Returns a StackWalker instance with the given options specifying..." "If the given ["options"|"set"] is empty, ..." (Looks like a typo ("@ocde") on StackWalker.java lines 278 & 307)

Fixed.

Code: == jdk/src/java.base/share/classes/sun/util/logging/PlatformLogger.java 550: The comment for get() states that it returns a StackTraceElement

Fixed.

== hotspot/src/share/vm/prims/jvm.h (219) jdk/src/java.base/share/native/include/jvm.h (196)

FWIW, the startindex argument of JVMFillStackFrames is an int in the hotspot file, and a jint in the jdk file.

Fixed.

== jdk/src/java.logging/share/classes/java/util/logging/LogRecord.java 658: The comment for get() states that it returns a StackTraceElement

Fixed.

== jdk/src/java.base/share/classes/java/lang/StackStreamFactory.java

137 // VM writes to these arrays to fill the stack frame information 138 // for each batch I think this comment applies to a previous version of the code.

195 * Subclass should override this method to change the batch size 196 * or invoke the function passed to the StackWalker::walk method I believe the function in question is the batchSizeMapper function, no longer being passed to walk(); line 196 can be removed.

Fixed.

== jdk/src/java.base/share/classes/java/lang/StackFrameInfo.java 66 Perhaps this.memberName does not need to be allocated in the case of -XX:-MemberNameInStackFrame ?

For more accurate footprint comparison, yes it should not allocate MemberName. Same for the injected fields if -XX:+MemberNameInStackFrame is set which is the default. I prefer to leave it as the follow-on work for JDK-8141239. This should give a good starting point to do performance measurement for Throwable.

Mandy

Thanks, -Brent On 11/16/15 4:13 PM, Mandy Chung wrote: I’d like to get the code review done by this week.

I renamed the static factory method from create to getInstance since “create” implies to create a new instance but the method returns a cached instance instead. I also changed the spec of getCallerClass per [1]. There is not much change since webrev.01. Webrev: http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02 javadoc: http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/ Mandy [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036589.html

More information about the hotspot-runtime-dev mailing list