Code Review for JEP 259: Stack-Walking API (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Wed Nov 18 22:59:52 UTC 2015

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

More information about the hotspot-runtime-dev mailing list