> On Nov 18, 2015, at 11:00 AM, Brent Christian <brent.christ...@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
>>
