Code Review for JEP 259: Stack-Walking API
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Nov 19 04:48:39 UTC 2015
I have a bit more. I'm still trying to figure out what this does.
If StackWalkAnchor is only used inside of stackwalk.cpp, the class
should be defined in the .cpp file.
StackWalkAnchor isn't a StackObj. It looks like a ValueObj (use macro
VALUE_OBJ_CLASS_SPEC)
I was surprised that StackWalkAnchor::decode_frames() had the job to
actually walk the stack and fill in the StackFrameInfo elements. It
seems like it's really a job of a function in StackWalk, not
StackWalkAnchor. I missed it completely in the first reading because I
don't think it's in the right place. The fact that it has to refer to
StackWalk:: in lots of places also indicates this.
It should be fill_in_frames or something like that.
create_primitive_value_instance
I think you can create a boxed object from inside the JVM much more
cheaply than doing an upcall to Java. Is this supposed to be fast at all?
See: oop java_lang_boxing_object::initialize_and_allocate(BasicType
type, TRAPS) {
http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02/jdk/src/java.base/share/classes/java/lang/LiveStackFrameInfo.java.html
Has rogue printf.
static PrimitiveValue asPrimitive(char value) {
System.out.println("CharPrimitive " + value);
return new CharPrimitive(value);
}
http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02/jdk/src/java.base/share/classes/java/lang/LiveStackFrame.java.html
My jdk8 memory of this is vague, but default methods have a footprint
and processing cost in the jvm because we have to check all the
overriding is legit after parsing classes with them and may end up
creating a new constant pool. I thought the real purpose of default
methods was to support adding methods to older interfaces that people
are already using.
120 default int intValue() {
121 throw new UnsupportedOperationException("this primitive of type " + type());
This could also throw AME.
I think this is enough. One comment below.
On 11/18/15 7:40 PM, Mandy Chung wrote:
> 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.
The confusion is that the user code doesn't really do stack walking, it
consumes the frames for whatever purpose it has. The JVM has already
"walked" the stack.
Not sure what the problem is if it reuses a stack stream. Is that bad?
> //
> // 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
> // :
How about shorter for less opportunity for confusion (and less comments
to change if something changes above).
// JVM_CallStackWalk walks the stack and fills in stack frames, then
calls to Java method
java.lang.StackStreamFactory.AbstractStackWalker::doStackWalk which
// calls the implementation to consume the stack frames.
// When JVM_CallStackWalk returns it invalidates the stack, which is I
guess what the anchor bit is for.
You don't need the stack below but it does make sense now that I've read
the code, but didn't before.
Thanks,
Coleen
>> 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
>
More information about the hotspot-runtime-dev
mailing list