Code Review for JEP 259: Stack-Walking API

[Coleen Phillimore]

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.

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 ?

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.

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

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?

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

                            TRAPS) {


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?

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

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.

   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,


AbstractStackWalker_klass not needed here anymore.

  100   static Klass*  AbstractStackWalker_klass(TRAPS);


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

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


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

Coleen


On 11/16/15 7: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