Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

> 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 start_index argument of
> JVM_FillStackFrames 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
>>