Code Review for JEP 259: Stack-Walking API

[Jaroslav Bachorik]

Hi,

On 10.11.2015 03:32, Mandy Chung wrote:
> javadoc:
>     http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
>
> webrev:
>    http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
>
> Overview of the implementation:
> When stack walking begins, the StackWalker calls into the VM to anchor a native frame (callStackWalk) that will fetch the first batch of stack frames.  VM will then invoke the doStackWalk method so that the consumer starts getting StackFrame object for each frame.  If all frames in the current batch are traversed, it will ask the VM to fetch the next batch.  The library side is doing the filtering of reflection frames.   For this patch, the VM filters of the hidden frames and also filter out Throwable::init related frames for stack trace.
>
> Ultimately we will move to these built-in logic out from the VM to the library but I’d like to separate them as future works.
>
> This patch also includes the change for Throwable to use StackWalker but it’s disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The VM backtrace is well tuned for performance.  So we separate the switch of Throwable to use StackWalker as a follow-on work:
>     JDK-8141239 Throwable should use StackWalker API to capture the backtrace
>
> MemberName initialization is one source of overhead and we propose to keep the VM flag -XX:-MemberNameInStackFrame for the time being for the performance work to continue for JDK-8141239.

Mostly nits and typos.

hotspot/src/share/vm/classfile/javaClasses.cpp
L1941 - variable 'cpref' is not used

hotspot/src/share/vm/classfile/javaClasses.inline.hpp
L117 - line number 'bci + 1000000' has a special meaning?

hotspot/src/share/vm/prims/stackwalk.cpp
L471 - typo 'pendiing' -> 'pending'

src/java.base/share/classes/java/lang/LiveStackFrameInfo.java
L40,L68 - typo '""liveStackFrames"' -> '"liveStackFrames"'
L65-66 - probably a redundant info (the exact permission is listed on L67-68

jdk/src/java.base/share/classes/java/lang/StackWalker.java
L222 - the documentation states only locals and operands while also 
monitors are to be collected

jdk/src/java.base/share/classes/java/lang/StackStreamFactory.java
L478-481 - should 'initialBatchSize' be >= START_POS ?
L574 - 'throw new IllegalStateException("origin " + origin + " != " + 
skipFrames);' -> 'throw new IllegalStateException("origin " + origin + " 
!= " + (skipFrames + START_POS));'

jdk/test/java/lang/StackWalker/VerifyStackTrace.java
L66,96,130 - 'the you can' -> 'you can'

There are several '// TODO' comments in the sources and tests - is there 
a plan to address them?

Cheers,

-JB-


>
> Thanks
> Mandy
>