Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

Coleen - thanks for the review. I’ll send webrev.03 tomorrow.

> On Nov 18, 2015, at 2:59 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> Hi,
> 
> Here are some actual comments on the code, with varying levels of detail and one higher level question.  It looks like stackwalk.cpp does what BacktraceBuilder in javaClasses does and pending Daniels change will work for redefinition.
> 
> In http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02/hotspot/src/share/vm/memory/universe.hpp.udiff.html
> 
> Could you rename
> 
> _stackWalker_callback_cache
> 
> Something like do_stack_walk_cache because the method is called doStackWalk?  The 'callback' is unnecessary in the name.
> 

Done.

> I filed an RFE to refactor some of this code in universe.cpp, since this adds the 4th instance of this pattern of code.
> 
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02/hotspot/src/share/vm/classfile/javaClasses.hpp.udiff.html
> 
> Can BackTrace be Backtrace ?

Done.

> 
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02/hotspot/src/share/vm/prims/stackwalk.cpp.html
> 
> The indentation of create_primitive_value_instance should be 2 not 4.
> 

Fixed.

> Also, this C++ isn't processed by javadoc so we don't need to have @argument boilerplate.
> 

OK. 

> So the Java code calls JVM_
> Handle StackWalk::walk(Handle stackStream, jlong mode,
> 
> and this function has an upcall to Java doStackWalk? then returns to Java?  Why not just return the argument to the Java code and it can call doStackWalk?


JVM_CallStackWalk is to anchor the stack so that we can ensure that the user code will walk the stack in a controlled manner.

This native method is to establish its own stack frame and then provide controlled access to the JVM's stack walking logic, for older frames. When this native method returns, we can invalidate the stack stream and reliably detect that a stream has been reused.  I added this comment.  Would it help?

  // call AbstractStackWalker::doStackWalk with the first batch of stack frames
  // for the user code to begin stack walking.
  //
  // JVM_CallStackWalk is the anchored frame to mark the stack walk begins
  // and the Java code can fetch subsequent batches of stack frames.
  // When JVM_CallStackWalk returns, the Java code will invalidate the StackFrame
  // stream and reliably detect if the traversed stack stream is being reused and
  // throw IllegalStateException.
  //
  // The stack looks like this:
  //
  // [n+4] implementation consuming StackFrame object
  // [n+3] java.lang.StackStreamFactory.AbstractStackWalker::doStackWalk
  // [n+2] JVM_CallStackWalk
  // [n+1] java.lang.StackWalker::walk
  // [n]   Java method
  //  :

> 
> In the new code the '{' belongs on the line with the method declarator.
> 
>                           TRAPS) {
> 

Fixed.

> 
> Is the TraceStackWalk printing useful for a customer, or is it left over debugging?  Should it be included with logging in the product? Is all of it useful?
> 

It’s for debugging.  I change it to develop flag.

> Can you make /* */ comments into // comments?
> 

OK.

> http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02/hotspot/src/share/vm/prims/stackwalk.hpp.html
> 
> This mode should be defined as JVM_STACKWALK_MODE sort of constants in both jvm.h files, not in stackwalk.hpp otherwise they will NOT stay in sync.
> 

Good point.  I’ll see what I can do.

>  70   enum {
>  71     magic_pos = 0,
>  72     fill_class_refs_only     = 0x2,
>  73     fill_method_name         = 0x4,
>  74     fill_frame_buffer_only   = 0x8,
>  75     filter_fillInStackTrace  = 0x10,
>  76     show_hidden_frames       = 0x20,
>  77     fill_locals_operands     = 0x100
>  78   };
> 
> Stackwalk::walk can return an oop and the caller can handleize it if necessary.  Only input parameters really need to be Handles.
> 
>  83   static Handle walk(Handle stackStream, jlong mode,
> 
> 

Fixed.

> AbstractStackWalker_klass not needed here anymore.
> 
> 100   static Klass*  AbstractStackWalker_klass(TRAPS);
> 

Fixed.

> 
> My knowledge of the Java side is limited.  It looks like the new stack walking code is used in logging.

Yes this is one use case for the stack walking API.  Current the logging has to ask for Throwable and filter out its own logging implementation frames and then find the true caller.   Logging doesn’t need to snapshot the entire stack trace.  With the stack walking API, it can traverse the top few frames and stop there once it finds the caller.

> 
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02/jdk/src/java.base/share/native/libjava/AbstractStackWalker.c.html
> 
> Why is there 0024 in this name?
> 
>  43 JNIEXPORT jobject JNICALL Java_java_lang_StackStreamFactory_00024AbstractStackWalker_callStackWalk
> 

Inner class - unicode for $.

> 
> It's nice that 2400 of the 6000 new lines of code are tests.
> 

Thanks to Daniel, Brent, and Hamlin for many new tests.

Mandy