Code Review for JEP 259: Stack-Walking API

[Coleen Phillimore]

Hi Mandy,
Thank you for making these changes.  I think it looks really good!

On 11/19/15 4:09 PM, Mandy Chung wrote:
> Hi Coleen,
>
> Here is the revised webrev incorporating your suggestions and a couple other minor java side change:
>     http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.03-delta
>
> I got some idea how to prepare Throwable to switch to StackWalker API to address the footprint/performance concern.  I agree with you that it would have to use mid/cpref in the short term for Throwable.  StackWalker::walk should use MemberName and should improve the footprint/performance issue in the long term.
>
> Are you okay to follow that up with JDK-8141239 and Throwable continues to use the VM backtrace until JDK-8141239 is resolved?

Yes, and as we discussed, I'd like to help with this.

Thanks!
Coleen
>
> Comments inlined below.
>
>> On Nov 18, 2015, at 8:48 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>>
>> 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)
>>
> It stores the vframeStream that is being traversed and the thread of the stack being traversed.  The address of this anchor is used to do the validation when any thread gets a hold of the Java object doing the stack walking and call into the VM (e.g. calling StackWalk::moreFrame from another thread),  it will check if the address of the anchor passed in is expected and the thread matches the current thread before it continues fetching the second batch of frames.
>
>> 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.
>>
> Good suggestion and it’s moved to StackWalk.
>
>> 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?
>>
> This is to create a PrimitiveValue object (not the boxed object). We should wait for feedback for this.
>
>> 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);
>>      }
>>
> Fixed.
>
>> 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());
> Oh.  I planned to clean this up.  It should be abstract class. Fixed.
>
>> This could also throw AME.
>>
>> I think this is enough.  One comment below.
>> :
>> 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?
> The stack may be changed already while accessing vFrameStream that is done stack walking.  Need to detect that.
>
>> 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.
> Fixed as you suggested.
>
> Mandy