Code Review for JEP 259: Stack-Walking API
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