Re: Code Review for JEP 259: Stack-Walking API

[serguei.spit...@oracle.com]

Somehow some of the formatting in my reply is gone.
I'm trying to fix it below...

Thanks,
Serguei


On 11/20/15 01:59, serguei.spit...@oracle.com wrote:

Hi Mandy,


It looks pretty good to me.
At least, I do not see any obvious issues.


Just some minor comments below.


webrev.03/hotspot/src/share/vm/classfile/javaClasses.cpp

2128 Symbol* java_lang_StackFrameInfo::get_file_name(Handle 
stackFrame, InstanceKlass* holder) {

2129   if (MemberNameInStackFrame) {
2130  return holder->source_file_name();
2131   } else {
2132  int bci = stackFrame->int_field(_bci_offset);
2133  short version = stackFrame->short_field(_version_offset);
2134
2135  return Backtrace::get_source_file_name(holder, version);
2136   }
2137 }

 The 'int bci' is not used above.


2139 void java_lang_StackFrameInfo::set_method_and_bci(Handle 
stackFrame, const methodHandle& method, int bci) {

2140   if (MemberNameInStackFrame) {
2141 oop mname = stackFrame->obj_field(_memberName_offset);
2142 InstanceKlass* ik = method->method_holder();
2143 CallInfo info(method(), ik);
2144 MethodHandles::init_method_MemberName(mname, info);
2145 java_lang_StackFrameInfo::set_bci(stackFrame(), bci);
2146 // method may be redefined; store the version
2147 int version = method->constants()->version();
2148 assert((jushort)version == version, "version should be short");
2149 java_lang_StackFrameInfo::set_version(stackFrame(), 
(short)version);

2150   } else {
2151 int mid = method->orig_method_idnum();
2152 int version = method->constants()->version();
2153 int cpref = method->name_index();
2154 assert((jushort)mid == mid, "mid should be short");
2155 assert((jushort)version == version, "version should be short");
2156 assert((jushort)cpref == cpref, "cpref should be short");
2157
2158 java_lang_StackFrameInfo::set_mid(stackFrame(), (short)mid);
2159 java_lang_StackFrameInfo::set_version(stackFrame(), 
(short)version);

2160 java_lang_StackFrameInfo::set_cpref(stackFrame(), (short)cpref);
2161 java_lang_StackFrameInfo::set_bci(stackFrame(), bci);
2162   }
2163 }

Minor: The calls to set_version() and set_bci() are the same for both 
alternatives
   and can be done just once before the if-else statement as 
below.

   With such refactoring it is clear what the common part is.

void java_lang_StackFrameInfo::set_method_and_bci(Handle stackFrame, 
const methodHandle& method, int bci) {
   java_lang_StackFrameInfo::set_bci(stackFrame(), bci); // method may 
be redefined; store the version

   int version = method->constants()->version();
   assert((jushort)version == version, "version should be short");
   java_lang_StackFrameInfo::set_version(stackFrame(), (short)version);
   if (MemberNameInStackFrame) {
 oop mname = stackFrame->obj_field(_memberName_offset);
 InstanceKlass* ik = method->method_holder();
 CallInfo info(method(), ik);
 MethodHandles::init_method_MemberName(mname, info);
  } else {
 int mid = method->orig_method_idnum();
 int cpref = method->name_index();
 assert((jushort)mid == mid, "mid should be short");
 assert((jushort)cpref == cpref, "cpref should be short");
 java_lang_StackFrameInfo::set_mid(stackFrame(), (short)mid);
 java_lang_StackFrameInfo::set_cpref(stackFrame(), (short)cpref);
  }
}


webrev.03/hotspot/src/share/vm/prims/jvm.cpp

572 int limit = start_index+frame_count; 597 int limit = 
start_index+frame_count;


Minor: Need spaces around the '+'. 
webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp



  62 // Parameters:
  63 //  thread.Current Java thread.
  64 //  magic. Magic value used for each stack walking
  65 //  classes_array  User-supplied buffers.  The 0th element is 
reserved

  . . .

  86 // Parameters:
  87 //   mode.  Restrict which frames to be decoded.
  88 //   decode_limit.  Maximum of frames to be decoded.
  89 //   start_index.   Start index to the user-supplied buffers.
  90 //   classes_array. Buffer to store classes in, starting at 
start_index.
  91 //   frames_array.  Buffer to store StackFrame in, starting at 
start_index.

  92 //  NULL if not used.
  93 //   end_index. End index to the user-supplied buffers with 
unpacked frames.

  94 //
  . . .
 274 // Parameters:
 275 //   stackStream.   StackStream object
 276 //   mode.  Stack walking mode.
 277 //   skip_frames.   Number of frames to be skipped.
 278 //   frame_count.   Number of frames to be traversed.
 279 //   start_index.   Start index to the user-supplied buffers.
 280 //   classes_array. Buffer to store classes in, starting at 
start_index.
 281 //   frames_array.  Buffer to store StackFrame in, starting at 
start_index.

 . . .
 414 // Parameters:
 415 //   stackStream.   StackStream object
 416 //   mode.  Stack walking mode.
 417 //   magic. Must be valid value to continue the stack walk
 418 //   frame_count.   

Re: Code Review for JEP 259: Stack-Walking API

[serguei.spit...@oracle.com]

Hi Mandy,


It looks pretty good to me.
At least, I do not see any obvious issues.


Just some minor comments below.


webrev.03/hotspot/src/share/vm/classfile/javaClasses.cpp

2128 Symbol* java_lang_StackFrameInfo::get_file_name(Handle stackFrame, 
InstanceKlass* holder) {

2129 if (MemberNameInStackFrame) {
2130 return holder->source_file_name();
2131 } else {
2132 int bci = stackFrame->int_field(_bci_offset);
2133 short version = stackFrame->short_field(_version_offset);
2134
2135 return Backtrace::get_source_file_name(holder, version);
2136 }
2137 }

 The 'int bci' is not used above.

2139 void java_lang_StackFrameInfo::set_method_and_bci(Handle 
stackFrame, const methodHandle& method, int bci) {

2140 if (MemberNameInStackFrame) {
2141 oop mname = stackFrame->obj_field(_memberName_offset);
2142 InstanceKlass* ik = method->method_holder();
2143 CallInfo info(method(), ik);
2144 MethodHandles::init_method_MemberName(mname, info);
2145 java_lang_StackFrameInfo::set_bci(stackFrame(), bci);
2146 // method may be redefined; store the version
2147 int version = method->constants()->version();
2148 assert((jushort)version == version, "version should be short");
2149 java_lang_StackFrameInfo::set_version(stackFrame(), (short)version);
2150 } else {
2151 int mid = method->orig_method_idnum();
2152 int version = method->constants()->version();
2153 int cpref = method->name_index();
2154 assert((jushort)mid == mid, "mid should be short");
2155 assert((jushort)version == version, "version should be short");
2156 assert((jushort)cpref == cpref, "cpref should be short");
2157
2158 java_lang_StackFrameInfo::set_mid(stackFrame(), (short)mid);
2159 java_lang_StackFrameInfo::set_version(stackFrame(), (short)version);
2160 java_lang_StackFrameInfo::set_cpref(stackFrame(), (short)cpref);
2161 java_lang_StackFrameInfo::set_bci(stackFrame(), bci);
2162 }
2163 } Minor: The calls to set_version() and set_bci() are the same for 
both alternatives and can be done just once before the if-else statement 
as below. With such refactoring it is clear what the common part is. 
void java_lang_StackFrameInfo::set_method_and_bci(Handle stackFrame, 
const methodHandle& method, int bci) { 
java_lang_StackFrameInfo::set_bci(stackFrame(), bci); // method may be 
redefined; store the version int version = 
method->constants()->version(); assert((jushort)version == version, 
"version should be short"); 
java_lang_StackFrameInfo::set_version(stackFrame(), (short)version); if 
(MemberNameInStackFrame) { oop mname = 
stackFrame->obj_field(_memberName_offset); InstanceKlass* ik = 
method->method_holder(); CallInfo info(method(), ik); 
MethodHandles::init_method_MemberName(mname, info); } else { int mid = 
method->orig_method_idnum(); int cpref = method->name_index(); 
assert((jushort)mid == mid, "mid should be short"); 
assert((jushort)cpref == cpref, "cpref should be short"); 
java_lang_StackFrameInfo::set_mid(stackFrame(), (short)mid); 
java_lang_StackFrameInfo::set_cpref(stackFrame(), (short)cpref); } }



webrev.03/hotspot/src/share/vm/prims/jvm.cpp

572 int limit = start_index+frame_count; 597 int limit = 
start_index+frame_count;


Minor: Need spaces around the '+'. 
webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp


  62 // Parameters:
  63 //  thread.Current Java thread.
  64 //  magic. Magic value used for each stack walking
  65 //  classes_array  User-supplied buffers.  The 0th element is reserved
  . . .

  86 // Parameters:
  87 //   mode.  Restrict which frames to be decoded.
  88 //   decode_limit.  Maximum of frames to be decoded.
  89 //   start_index.   Start index to the user-supplied buffers.
  90 //   classes_array. Buffer to store classes in, starting at start_index.
  91 //   frames_array.  Buffer to store StackFrame in, starting at start_index.
  92 //  NULL if not used.
  93 //   end_index. End index to the user-supplied buffers with unpacked 
frames.
  94 //
  . . .
 274 // Parameters:
 275 //   stackStream.   StackStream object
 276 //   mode.  Stack walking mode.
 277 //   skip_frames.   Number of frames to be skipped.
 278 //   frame_count.   Number of frames to be traversed.
 279 //   start_index.   Start index to the user-supplied buffers.
 280 //   classes_array. Buffer to store classes in, starting at start_index.
 281 //   frames_array.  Buffer to store StackFrame in, starting at start_index.
 . . .
 414 // Parameters:
 415 //   stackStream.   StackStream object
 416 //   mode.  Stack walking mode.
 417 //   magic. Must be valid value to continue the stack walk
 418 //   frame_count.   Number of frames to be decoded.
 419 //   start_index.   Start index to the user-supplied buffers.
 420 //   classes_array. Buffer to store classes in, starting at start_index.
 421 //   frames_array.  Buffer to store StackFrame in, starting at start_index.

  Minor: Dots after the parameter names looks strange.  
Probably better to remove them or 

Re: Code Review for JEP 259: Stack-Walking API

[serguei.spit...@oracle.com]

On 11/20/15 08:39, Mandy Chung wrote:

On Nov 20, 2015, at 1:59 AM, serguei.spit...@oracle.com wrote:

  The 'int bci' is not used above.

This is weird.   Daniel caught that and I took that line out already.
http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.03-delta/hotspot/src/share/vm/classfile/javaClasses.cpp.sdiff.html

Anyway this webrev is the up-to-date one including fixing the nits you pointed 
out.
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.04


   Minor: The calls to set_version() and set_bci() are the same for both 
alternatives
  and can be done just once before the if-else statement as below.
  With such refactoring it is clear what the common part is.


Moved.


Thanks.





webrev.03/hotspot/src/share/vm/prims/jvm.cpp

Minor: Need spaces around the '+'. 
webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp

I am not sure if that’s the convention applied consistently in VM.  I fixed it 
anyway.


webrev.04/hotspot/src/share/vm/prims/jvm.cpp

  One place left with inconsistent formatting:

597 int limit = start_index+frame_count;




   Minor: Indent at the line 115 must be +2.

I don’t see any indentation/formatting issue there.

   I see it fixed in new webrev. :)



  360 for (int n=0; n < skip_frames && !vfst.at_end(); vfst.next(), n++) {


I prefer to keep n=0 and there are other places using that convention.

  Ok.



  451   int count = frame_count+start_index;
   Minor: Need spaces around the '=' and '+’.

Fixed.
Thank you for making the changes! I do not need another webrev. Amazing 
work in general! Thanks, Serguei


Thanks
Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Daniel Fuchs]

On 20/11/15 03:13, Mandy Chung wrote:

Cross-posting to core-libs-dev.

Delta webrev incorporating feedback from Coleen and Brent and also a fix that 
Daniel made:
http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.03-delta/

Full webrev:
http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.03/

This also includes a couple of new tests Hamlin has contributed.


Looks good to me Mandy!

-- daniel


Mandy


On Nov 19, 2015, at 1:38 PM, Coleen Phillimore  
wrote:


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

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 20, 2015, at 1:59 AM, serguei.spit...@oracle.com wrote:
> 
>  The 'int bci' is not used above.

This is weird.   Daniel caught that and I took that line out already.
http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.03-delta/hotspot/src/share/vm/classfile/javaClasses.cpp.sdiff.html

Anyway this webrev is the up-to-date one including fixing the nits you pointed 
out.
  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.04

> 
>   Minor: The calls to set_version() and set_bci() are the same for both 
> alternatives
>  and can be done just once before the if-else statement as below.
>  With such refactoring it is clear what the common part is.
> 

Moved.

> 
> webrev.03/hotspot/src/share/vm/prims/jvm.cpp
> 
> Minor: Need spaces around the '+'. 
> webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp

I am not sure if that’s the convention applied consistently in VM.  I fixed it 
anyway.

> 
>   Minor: Indent at the line 115 must be +2.

I don’t see any indentation/formatting issue there.

>  360 for (int n=0; n < skip_frames && !vfst.at_end(); vfst.next(), n++) {
> 

I prefer to keep n=0 and there are other places using that convention.

>  451   int count = frame_count+start_index;
>   Minor: Need spaces around the '=' and '+’.

Fixed.

Thanks
Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

Cross-posting to core-libs-dev.

Delta webrev incorporating feedback from Coleen and Brent and also a fix that 
Daniel made:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.03-delta/

Full webrev:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.03/

This also includes a couple of new tests Hamlin has contributed.

Mandy

> On Nov 19, 2015, at 1:38 PM, Coleen Phillimore  
> wrote:
> 
> 
> 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

Re: Code Review for JEP 259: Stack-Walking API

[Brent Christian]

Hi, Mandy

I looked through the jdk code.  I think it's looking quite good.  I just 
noticed some small details to consider fixing up before pushing.


Docs:

StackWalker.StackFrame.getClassName():
the "binary name" link seems to be broken
(StackWalker.java line 100)

StackWalker.getInstance(Set options):
StackWalker.getInstance(Set options, int estimateDepth):

A couple missing words:

"Returns a StackWalker instance with the given *options* specifying..."

"If the given *["options"|"set"]* is empty, ..."

(Looks like a typo ("@ocde") on StackWalker.java lines 278 & 307)


Code:

==
jdk/src/java.base/share/classes/sun/util/logging/PlatformLogger.java

550:
The comment for get() states that it returns a StackTraceElement

==
hotspot/src/share/vm/prims/jvm.h (219)
jdk/src/java.base/share/native/include/jvm.h (196)

FWIW, the start_index argument of
JVM_FillStackFrames is an int in the hotspot file, and a jint in the jdk 
file.


==
jdk/src/java.logging/share/classes/java/util/logging/LogRecord.java

658:
The comment for get() states that it returns a StackTraceElement

==
jdk/src/java.base/share/classes/java/lang/StackStreamFactory.java

 137 // VM writes to these arrays to fill the stack frame 
information

 138 // for each batch

I think this comment applies to a previous version of the code.


 195  * Subclass should override this method to change the 
batch size
 196  * or invoke the function passed to the StackWalker::walk 
method


I believe the function in question is the batchSizeMapper function, no 
longer being passed to walk(); line 196 can be removed.


==
jdk/src/java.base/share/classes/java/lang/StackFrameInfo.java

66
Perhaps this.memberName does not need to be allocated in the case of 
-XX:-MemberNameInStackFrame ?


Thanks,
-Brent

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

Re: Code Review for JEP 259: Stack-Walking API

[Coleen Phillimore]

On 11/18/15 5:06 PM, Mandy Chung wrote:

On Nov 18, 2015, at 1:01 PM, Coleen Phillimore  
wrote:


One of the things that I'm struggling with is that StackFrameInfo contains both 
the collected information from walking the stack frames, method id, bci, 
mirror, version and cpref, and the digested information: interned string for 
class name, method name, line number and source file name.


method id, mirror, version and cpref are injected in StackFrameInfo.  I hope 
there is a way to make it conditional only if -XX:-MemberNameInThrowable is set 
(is it possible?)


That's really hard to do with the nice macros we have now in javaClasses.


-XX:-MemberNameInThrowable is temporary and disabled by default.  It is used 
for follow-on performance improvement as we discussed previously.   I still 
believe that there may be some low hanging fruit to reduce the initialization 
of MemberName for an already-linked method.We should aim to remove this 
flag in JDK 9 so that StackFrameInfo will have only MemberName and bci.


Given that that we were trying to stick to the original feature freeze 
date for 9, I don't think the performance of the MethodHandles code 
would make it in time.  There are some big problems with it, notably 
that it creates weak references for each MemberName and the GC people 
are not going to like that.   We have not to-date found a better 
solution for this to support redefinition.


I think if StackFrameInfo was trimmed to just what was needed for 
collecting the information during stack traces, it would be possible to 
make the new implementation performant enough to be low risk for 9 *and* 
would allow for removing the duplicated code in BacktraceBuilder.  This 
would be a very promising improvement!


The interned string for class name, method name, line number and source file 
name are requested lazily when StackFrame::getMethodName or other methods are 
called.  They are not eagerly allocated.


But the fields in StackFrameInfo are present for each element in the 
stack trace.  We had problems with GC scavenge times when we increased 
the size of the backtrace that we collect today.   The StackFrameInfo 
struct would be similarly sized if you didn't all the fields from 
StackTraceElement, so it would be good.



If this is to replace stack walking for exceptions, this will more than double 
the footprint of the information collected but rarely used. I don't understand 
why the digested information isn't still StackFrameElement?


If Throwable uses StackWalker, I expect it to use MemberName and 
-XX:-MemberNameInThrowable should be removed by that time.   Also VM no longer 
needs to fill in StackTraceElement as it should fill in StackFrameInfo.   
java_lang_StackTraceElement in javaClasses.[hc]pp can be removed at an 
appropriate time :)


I don't know why StackTraceElement should be removed.  What's wrong with 
StackTraceElement?


Throwable backtrace will keep an array of StackFrameInfo, one element per 
frame.  StackFrameInfo only captures the MemberName + bci.


Right (or the combination of things that we save now in the backtraces 
for efficiency).




When Throwable::getStackTraceElements() or printStackTrace() is called, the VM 
will allocate the intern strings for those names and fill in StackFrameInfo.  
Then convert them to StackTraceElement[] and throw away StackFrameInfo[].   
This is just the current implementation.   I expect further optimization can be 
done in the JDK side about StackTraceElement and StackFrameInfo.


This sounds really inefficient!  Why not fill in StackTraceElement 
directly?  And keep it?


Even in Java, having one class represent two different things isn't very 
object oriented.



That's just a high level comment.  I haven't read the java code yet for the 
rationale why this type is used for two different things.


The way I implement it is to prepare Throwable backtrace + StackTraceElement be 
replaced by StackFrameInfo in the VM.

The VM fills in StackFrameInfo for StackWalker.   When Throwable switches to 
use StackWalker, VM doesn’t need to know anything about StackTraceElement.


I don't see the advantage of this. 
java_lang_StackFrameInfo::fill_methodInfo() could just fill in a Java 
allocated array of StackTraceElement instead.  Again, making 
StackFrameInfo so large will have footprint and GC performance 
implications when it's almost always thrown away.  I included GC in the 
mailing list.  Hopefully with enough context if they want to comment.


thanks,
Coleen



Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 18, 2015, at 1:01 PM, Coleen Phillimore  
> wrote:
> 
> 
> One of the things that I'm struggling with is that StackFrameInfo contains 
> both the collected information from walking the stack frames, method id, bci, 
> mirror, version and cpref, and the digested information: interned string for 
> class name, method name, line number and source file name.
> 

method id, mirror, version and cpref are injected in StackFrameInfo.  I hope 
there is a way to make it conditional only if -XX:-MemberNameInThrowable is set 
(is it possible?)

-XX:-MemberNameInThrowable is temporary and disabled by default.  It is used 
for follow-on performance improvement as we discussed previously.   I still 
believe that there may be some low hanging fruit to reduce the initialization 
of MemberName for an already-linked method.We should aim to remove this 
flag in JDK 9 so that StackFrameInfo will have only MemberName and bci.

The interned string for class name, method name, line number and source file 
name are requested lazily when StackFrame::getMethodName or other methods are 
called.  They are not eagerly allocated.

> If this is to replace stack walking for exceptions, this will more than 
> double the footprint of the information collected but rarely used. I don't 
> understand why the digested information isn't still StackFrameElement?
> 

If Throwable uses StackWalker, I expect it to use MemberName and 
-XX:-MemberNameInThrowable should be removed by that time.   Also VM no longer 
needs to fill in StackTraceElement as it should fill in StackFrameInfo.   
java_lang_StackTraceElement in javaClasses.[hc]pp can be removed at an 
appropriate time :)

Throwable backtrace will keep an array of StackFrameInfo, one element per 
frame.  StackFrameInfo only captures the MemberName + bci.  When 
Throwable::getStackTraceElements() or printStackTrace() is called, the VM will 
allocate the intern strings for those names and fill in StackFrameInfo.  Then 
convert them to StackTraceElement[] and throw away StackFrameInfo[].   This is 
just the current implementation.   I expect further optimization can be done in 
the JDK side about StackTraceElement and StackFrameInfo.

> That's just a high level comment.  I haven't read the java code yet for the 
> rationale why this type is used for two different things.
> 

The way I implement it is to prepare Throwable backtrace + StackTraceElement be 
replaced by StackFrameInfo in the VM.

The VM fills in StackFrameInfo for StackWalker.   When Throwable switches to 
use StackWalker, VM doesn’t need to know anything about StackTraceElement.

Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Brent Christian]

On 11/18/15 2:24 PM, Mandy Chung wrote:

==
>jdk/src/java.base/share/classes/java/lang/StackFrameInfo.java
>
>66
>Perhaps this.memberName does not need to be allocated in the case of 
-XX:-MemberNameInStackFrame ?
>

For more accurate footprint comparison, yes it should not allocate MemberName.  
Same for the injected fields if -XX:+MemberNameInStackFrame is set which is the 
default.  I prefer to leave it as the follow-on work for JDK-8141239.  This 
should give a good starting point to do performance measurement for Throwable.


That's fine with me.

Thanks,
-Brent

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 18, 2015, at 11:00 AM, Brent Christian  
> wrote:
> 
> Hi, Mandy
> 
> I looked through the jdk code.  I think it's looking quite good.  I just 
> noticed some small details to consider fixing up before pushing.
> 
> Docs:
> 
> StackWalker.StackFrame.getClassName():
> the "binary name" link seems to be broken
> (StackWalker.java line 100)
> 

It works for me.  It looks correct to me.

> StackWalker.getInstance(Set options):
> StackWalker.getInstance(Set options, int estimateDepth):
> 
> A couple missing words:
> 
> "Returns a StackWalker instance with the given *options* specifying..."
> 
> "If the given *["options"|"set"]* is empty, ..."
> 
> (Looks like a typo ("@ocde") on StackWalker.java lines 278 & 307)
> 

Fixed.

> 
> Code:
> 
> ==
> jdk/src/java.base/share/classes/sun/util/logging/PlatformLogger.java
> 
> 550:
> The comment for get() states that it returns a StackTraceElement
> 

Fixed.

> ==
> hotspot/src/share/vm/prims/jvm.h (219)
> jdk/src/java.base/share/native/include/jvm.h (196)
> 
> FWIW, the start_index argument of
> JVM_FillStackFrames is an int in the hotspot file, and a jint in the jdk file.

Fixed.

> 
> ==
> jdk/src/java.logging/share/classes/java/util/logging/LogRecord.java
> 
> 658:
> The comment for get() states that it returns a StackTraceElement
> 

Fixed.

> ==
> jdk/src/java.base/share/classes/java/lang/StackStreamFactory.java
> 
> 137 // VM writes to these arrays to fill the stack frame information
> 138 // for each batch
> 
> I think this comment applies to a previous version of the code.
> 
> 
> 195  * Subclass should override this method to change the batch size
> 196  * or invoke the function passed to the StackWalker::walk method
> 
> I believe the function in question is the batchSizeMapper function, no longer 
> being passed to walk(); line 196 can be removed.

Fixed.

> 
> ==
> jdk/src/java.base/share/classes/java/lang/StackFrameInfo.java
> 
> 66
> Perhaps this.memberName does not need to be allocated in the case of 
> -XX:-MemberNameInStackFrame ?
> 

For more accurate footprint comparison, yes it should not allocate MemberName.  
Same for the injected fields if -XX:+MemberNameInStackFrame is set which is the 
default.  I prefer to leave it as the follow-on work for JDK-8141239.  This 
should give a good starting point to do performance measurement for Throwable.  

Mandy

> 
> Thanks,
> -Brent
> 
> On 11/16/15 4: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
>> 

Re: Code Review for JEP 259: Stack-Walking API

[Coleen Phillimore]

One of the things that I'm struggling with is that StackFrameInfo 
contains both the collected information from walking the stack frames, 
method id, bci, mirror, version and cpref, and the digested information: 
interned string for class name, method name, line number and source file 
name.


If this is to replace stack walking for exceptions, this will more than 
double the footprint of the information collected but rarely used.  I 
don't understand why the digested information isn't still StackFrameElement?


That's just a high level comment.  I haven't read the java code yet for 
the rationale why this type is used for two different things.


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

Re: Code Review for JEP 259: Stack-Walking API

[Peter Levart]

Hi Mandy,

On 11/17/2015 01:13 AM, 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


Just read the javadoc so-far...

There are several mistakes in getCallerClass() API Note (some probably a 
left-over from previous iterations of the API):


 471  *  If this {@code getCallerClass} method is called by the 
entry point
 472  * of a thread, the {@code Class} of the method invoked at 
thread's start

 473  * time will be returned.

Hm, I can't decipher that. What about something like:

If this {@code getCallerClass} method is called by the entry point of a 
thread - a method overriding {@link Thread#run} (that's the only 
possibility),
the declaring class of the method overriding {@link Thread#run} will be 
returned.


Or, if you accept my latest suggestion:

If this {@code getCallerClass} method is called by the entry point of a 
thread - a method overriding {@link Thread#run},

{@link IllegalArgumentException} will be thrown.


 474  *
 475  * @apiNote
 476  * For example, {@code ResourceBundleUtil::getBundle} loads a 
resource bundle
 477  * on behalf of the caller.  It calls this {@code 
getCallerClass} method
 478  * to find the method calling {@code Util::getResourceBundle} 
and use the caller's
 479  * class loader to load the resource bundle. The caller class 
in this example

 480  * is the {@code MyTool} class.
 481  *
 482  * {@code
 483  * class ResourceBundleUtil {
 484  * private final StackWalker walker = 
StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);

 485  * public ResourceBundle getBundle(String bundleName) {
 486  * Class caller = walker.getCallerClass();
 487  * return ResourceBundle.getBundle(bundleName, 
caller.getClassLoader());

 488  * }
 489  * }
 490  *
 491  * class MyTool {
 492  * private void init() {
 493  * ResourceBundle rb = 
Util.getResourceBundle("mybundle");

 494  * }
 495  * }
 496  * }

- threre's no ResourceBundle.getBundle(String, ClassLoader) method.
- Util -> ResourceBundleUtil (or ResourceBundleUtil -> Util)


 497  *
 498  * An equivalent way to find the caller class using the
 499  * {@link StackWalker#walk walk} method is as follows
 500  * (filtering the reflection frames, {@code MethodHandle} and 
hidden frames

 501  * not shown below):
 502  * {@code
 503  * Class caller = walker.walk(s ->
 504  * s.map(StackFrame::getDeclaringClass)
 505  *  .skip(2)
 506  *  .findFirst());
 507  * }

- Stream.findFirst() returns Optional, not E.

 508  *
 509  * When the {@code getCallerClass} method is invoked from 
thread {@code t}'s
 510  * entry point, i.e. {@code PrimeRun::run}, it returns {@code 
PrimeRun} class
 511  * that is the first stack frame below the stack frame for 
{@code getCallerClass}

 512  * instead.
 513  *
 514  * {@code
 515  * class PrimeRun implements Runnable {
 516  * public void run() {
 517  * Class c = 
StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE)

 518  * .getCallerClass();
 519  * }
 520  * }
 521  * Thread t = new Thread(new PrimeRun()).start();
 522  * }
 523  *
 524  * Similarly, when the {@code getCallerClass} method is called 
from the
 525  * {@code static public void main} method launched by the 
{@code java} launcher,

 526  * it returns the {@code Class} of the {@code main} method.
 527  *
 528  * @return {@code Class} object of the caller's caller 
invoking this method;
 529  * or the {@code Class} object of the method invoked by a 
thread at start time.


- In above example, t's entry point is Thread::run (not PrimeRun::run). 
Thread::run then delegates to PrimeRun::run:


public class Thread {
...
public void run() {
if (target != null) {
target.run();
}
}


...so this example is not really suitable to describe the effect of 
invoking getCallerClass from thread's entry-point.


An example of that kind would be something like:

class PrimeThread extends Thread {
@Override public void run() {
Class c = StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE)
.getCallerClass();
}
}

Thread t = new 

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

Thanks Peter.  

> - threre's no ResourceBundle.getBundle(String, ClassLoader) method.
> - Util -> ResourceBundleUtil (or ResourceBundleUtil -> Util)
> 

Fixed.

> :
> - Stream.findFirst() returns Optional, not E.
> 

Fixed.

I updated javadoc for getCallerClass per our discussion.

/**
 * Gets the {@code Class} object of the caller invoking the method
 * that calls this {@code getCallerClass} method.
 *
 *  Reflection frames, {@link java.lang.invoke.MethodHandle} and
 * hidden frames are filtered regardless of the
 * {@link Option#SHOW_REFLECT_FRAMES SHOW_REFLECT_FRAMES}
 * and {@link Option#SHOW_HIDDEN_FRAMES SHOW_HIDDEN_FRAMES} options
 * if this {@code StackWalker} has been configured.
 *
 *  This method throws {@code UnsupportedOperationException} if
 * this {@code StackWalker} is not configured with
 * {@link Option#RETAIN_CLASS_REFERENCE RETAIN_CLASS_REFERENCE} option
 * or this method is called from the last frame on the stack.
 *
 * @apiNote
 * For example, {@code Util::getResourceBundle} loads a resource bundle
 * on behalf of the caller.  It calls this {@code getCallerClass} method
 * to find the method calling {@code Util::getResourceBundle} and use the 
caller's
 * class loader to load the resource bundle. The caller class in this example
 * is the {@code MyTool} class.
 *
 * {@code
 * class Util {
 * private final StackWalker walker = 
StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
 * public ResourceBundle getResourceBundle(String bundleName) {
 * Class caller = walker.getCallerClass();
 * return ResourceBundle.getBundle(bundleName, Locale.getDefault(), 
caller.getClassLoader());
 * }
 * }
 *
 * class MyTool {
 * private final Util util = new Util();
 * private void init() {
 * ResourceBundle rb = util.getResourceBundle("mybundle");
 * }
 * }
 * }
 *
 * An equivalent way to find the caller class using the
 * {@link StackWalker#walk walk} method is as follows
 * (filtering the reflection frames, {@code MethodHandle} and hidden frames
 * not shown below):
 * {@code
 * Optional caller = walker.walk(s ->
 * s.map(StackFrame::getDeclaringClass)
 *  .skip(2)
 *  .findFirst());
 * }
 *
 * When the {@code getCallerClass} method is called from a method that
 * is the last frame on the stack,
 * for example, {@code static public void main} method launched by the
 * {@code java} launcher or a method invoked from a JNI attached thread.
 * {@code UnsupportedOperationException} is thrown.
 *
 * @return {@code Class} object of the caller's caller invoking this method.
 *
 * @throws UnsupportedOperationException if this {@code StackWalker}
 * is not configured with {@link Option#RETAIN_CLASS_REFERENCE
 * Option.RETAIN_CLASS_REFERENCE}.
 * @throws UnsupportedOperationException if there is no caller frame, i.e.
 * when this {@code getCallerClass} method is called from a method
 * which is the last frame on the stack.
 */

Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

On Nov 17, 2015, at 4:00 PM, Ian Rogers  wrote:
> 
> Should the StackWalker.StackFrame interface provide a way to get the 
> java.lang.reflect.Method/Constructor/Member?
> 

Not in the scope of this JEP.

Mandy

> Thanks,
> Ian
> 
> On Tue, Nov 17, 2015 at 3:56 PM, Mandy Chung  wrote:
> Thanks Peter.
> 
> > - threre's no ResourceBundle.getBundle(String, ClassLoader) method.
> > - Util -> ResourceBundleUtil (or ResourceBundleUtil -> Util)
> >
> 
> Fixed.
> 
> > :
> > - Stream.findFirst() returns Optional, not E.
> >
> 
> Fixed.
> 
> I updated javadoc for getCallerClass per our discussion.
> 
> /**
>  * Gets the {@code Class} object of the caller invoking the method
>  * that calls this {@code getCallerClass} method.
>  *
>  *  Reflection frames, {@link java.lang.invoke.MethodHandle} and
>  * hidden frames are filtered regardless of the
>  * {@link Option#SHOW_REFLECT_FRAMES SHOW_REFLECT_FRAMES}
>  * and {@link Option#SHOW_HIDDEN_FRAMES SHOW_HIDDEN_FRAMES} options
>  * if this {@code StackWalker} has been configured.
>  *
>  *  This method throws {@code UnsupportedOperationException} if
>  * this {@code StackWalker} is not configured with
>  * {@link Option#RETAIN_CLASS_REFERENCE RETAIN_CLASS_REFERENCE} option
>  * or this method is called from the last frame on the stack.
>  *
>  * @apiNote
>  * For example, {@code Util::getResourceBundle} loads a resource bundle
>  * on behalf of the caller.  It calls this {@code getCallerClass} method
>  * to find the method calling {@code Util::getResourceBundle} and use the 
> caller's
>  * class loader to load the resource bundle. The caller class in this example
>  * is the {@code MyTool} class.
>  *
>  * {@code
>  * class Util {
>  * private final StackWalker walker = 
> StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
>  * public ResourceBundle getResourceBundle(String bundleName) {
>  * Class caller = walker.getCallerClass();
>  * return ResourceBundle.getBundle(bundleName, 
> Locale.getDefault(), caller.getClassLoader());
>  * }
>  * }
>  *
>  * class MyTool {
>  * private final Util util = new Util();
>  * private void init() {
>  * ResourceBundle rb = util.getResourceBundle("mybundle");
>  * }
>  * }
>  * }
>  *
>  * An equivalent way to find the caller class using the
>  * {@link StackWalker#walk walk} method is as follows
>  * (filtering the reflection frames, {@code MethodHandle} and hidden frames
>  * not shown below):
>  * {@code
>  * Optional caller = walker.walk(s ->
>  * s.map(StackFrame::getDeclaringClass)
>  *  .skip(2)
>  *  .findFirst());
>  * }
>  *
>  * When the {@code getCallerClass} method is called from a method that
>  * is the last frame on the stack,
>  * for example, {@code static public void main} method launched by the
>  * {@code java} launcher or a method invoked from a JNI attached thread.
>  * {@code UnsupportedOperationException} is thrown.
>  *
>  * @return {@code Class} object of the caller's caller invoking this method.
>  *
>  * @throws UnsupportedOperationException if this {@code StackWalker}
>  * is not configured with {@link Option#RETAIN_CLASS_REFERENCE
>  * Option.RETAIN_CLASS_REFERENCE}.
>  * @throws UnsupportedOperationException if there is no caller frame, i.e.
>  * when this {@code getCallerClass} method is called from a method
>  * which is the last frame on the stack.
>  */
> 
> Mandy
> 

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 17, 2015, at 10:42 AM, Daniel Fuchs  wrote:
> 
> On 17/11/15 01:13, 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
>> 
> 
> Looks good to me Mandy.
> Though I wonder whether we really need this optimization.

Thanks for the review.

Caching of StackWalker instances is not highly needed and I want to avoid the 
method name to preclude such possibility.

Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Daniel Fuchs]

On 17/11/15 01:13, 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



Looks good to me Mandy.
Though I wonder whether we really need this optimization.

best regards,

-- daniel

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 16, 2015, at 1:36 AM, Daniel Fuchs  wrote:
> 
> Hi Mandy,
> 
> Sorry I was not clear.
> I'm proposing the following changes:
> 
> StackFrameInfo.java:
> 
> 100 public OptionalInt getLineNumber() {
> 101 ensureMethodInfoInitialized();
> 102 return lineNumber != -1 &&  lineNumber != -2
>   ? OptionalInt.of(lineNumber)
>   : OptionalInt.empty();
> 103 }
> 

I see.  Thanks for catching it.

> StackWalker.java:
> 
> 175 public default StackTraceElement toStackTraceElement() {
> 176 return new StackTraceElement(getClassName(),
> getMethodName(),
> 177  getFileName().orElse(null),
> 178  getLineNumber()
> .orElse(isNativeMethod() ? -2 : -1));
> 179 }

That’s right. 

Mandy

Re: Code Review for JEP 259: Stack-Walking API

[David M. Lloyd]

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


I'll give the API a (very unofficial) "looks great".

--
- DML

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

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

Re: Code Review for JEP 259: Stack-Walking API

[Daniel Fuchs]

Hi Mandy,

Sorry I was not clear.
I'm proposing the following changes:

StackFrameInfo.java:

100 public OptionalInt getLineNumber() {
101 ensureMethodInfoInitialized();
102 return lineNumber != -1 &&  lineNumber != -2
   ? OptionalInt.of(lineNumber)
   : OptionalInt.empty();
103 }

StackWalker.java:

175 public default StackTraceElement toStackTraceElement() {
176 return new StackTraceElement(getClassName(),
 getMethodName(),
177  getFileName().orElse(null),
178  getLineNumber()
 .orElse(isNativeMethod() ? -2 : -1));
179 }

best regards,

-- daniel

On 13/11/15 18:58, Mandy Chung wrote:



On Nov 13, 2015, at 9:55 AM, Daniel Fuchs  wrote:

Hi Mandy,

StackFrameInfo.java:

100 public OptionalInt getLineNumber() {
101 ensureMethodInfoInitialized();
102 return lineNumber != -1 ? OptionalInt.of(lineNumber) : 
OptionalInt.empty();
103 }

If lineNumber is -2 then empty should probably be returned too.


lineNumber == -2 is used to indicate a native method.

-1 is unavailable.



Which means that the following code code in StackWalker.StackFrame
would need to be changed accordingly:

175 public default StackTraceElement toStackTraceElement() {
176 return new StackTraceElement(getClassName(), getMethodName(),
177  getFileName().orElse(null),
178  getLineNumber().orElse(-1));
179 }

The other option would be to document -2 as a special value
in StackFrame.getLineNumber() but that would be ugly?



StackTraceElement constructor takes -2 line number for native method.

Mandy


best regards

-- daniel

-- daniel

On 13/11/15 16:21, Mandy Chung wrote:

A revised webrev:
  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.01

javadoc:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

The main change in this version is mostly in the hotspot change to incorporate 
Coleen’s suggestion:
1. Move the new Klasses to systemDictionary as well known classes.
2. Cache the doStackWalker method
3. Move StackFrameInfo::mid, version, cpref fields as VM injected fields as 
they are not needed by Java.
These fields are interim for performance measurement to compare the cost of 
MemberName initialization.

The microbenchmark running StackWalker::walk shows 3-4.5X slower due to the use 
of MemberName.  I suspect the overhead might be due to the MemberName 
initialization has to handle unresolved and unlinked methods.  For stack 
walker, the method has been linked and it may be a low hanging fruit to fix in 
the VM.

4. Updates the javadoc of StackWalker::getCallerClass to reflect filtering of 
reflection and MethodHandle frames.

Open question:
- is it useful to have an option to configure filtering/showing 
java.lang.invoke frames?   A user can filter it themselves and I don’t think 
it’s highly needed and we can add it in the future if needed.

Mandy



On Nov 9, 2015, at 6:32 PM, Mandy Chung  wrote:

javadoc:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

webrev:
  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/

Overview of the implementation:
When stack walking begins, the StackWalker calls into the VM to anchor a native 
frame (callStackWalk) that will fetch the first batch of stack frames.  VM will 
then invoke the doStackWalk method so that the consumer starts getting 
StackFrame object for each frame.  If all frames in the current batch are 
traversed, it will ask the VM to fetch the next batch.  The library side is 
doing the filtering of reflection frames.   For this patch, the VM filters of 
the hidden frames and also filter out Throwable::init related frames for stack 
trace.

Ultimately we will move to these built-in logic out from the VM to the library 
but I’d like to separate them as future works.

This patch also includes the change for Throwable to use StackWalker but it’s 
disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The VM 
backtrace is well tuned for performance.  So we separate the switch of 
Throwable to use StackWalker as a follow-on work:
   JDK-8141239 Throwable should use StackWalker API to capture the backtrace

MemberName initialization is one source of overhead and we propose to keep the 
VM flag -XX:-MemberNameInStackFrame for the time being for the performance work 
to continue for JDK-8141239.

Thanks
Mandy

RE: Code Review for JEP 259: Stack-Walking API

[Timo Kinnunen]

The metadata shown by jstack is available internally too, albeit getting access 
to it is a bit convoluted. Also using it for anything other than printing it 
out requires parsing it, as it’s in an undocumented text format.


 

Sent from Mail for Windows 10



From: David M. Lloyd
Sent: Thursday, November 12, 2015 17:51
To: core-libs-dev@openjdk.java.net
Subject: Re: Code Review for JEP 259: Stack-Walking API


One other kind of stack metadata that is missing (yet is present with 
external tools such as jstack etc.) is ancillary data about stack frames 
indicating whether the frame holds or is waiting on a lock.  I'm not 
sure whether it's practical to add all this stuff though, 
performance-wise, unless maybe it is made optional with an option flag.

On 11/12/2015 09:29 AM, Jason Mehrens wrote:
> Mandy,
>
> Forgive my ignorance on this subject and impacts but, would it be possible to 
> include more metadata methods in the StackWalker.StackFrame interface such as 
> getParameterTypes/parameterArray and getReturnType/returnType?  Ideally, I 
> really would like these methods for StackTraceElement since knowing the exact 
> parameter types allows the caller to lookup the method to examine all the 
> method metadata which is useful for logging not only in output but for use in 
> stacktrace reduction algorithms.
>
> Thanks,
>
> Jason
>
> 
> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of 
> Mandy Chung <mandy.ch...@oracle.com>
> Sent: Monday, November 9, 2015 8:32 PM
> To: core-libs-dev; hotspot-runtime-...@openjdk.java.net
> Subject: Code Review for JEP 259: Stack-Walking API
>
> javadoc:
> 
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
>
> webrev:
>http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
>
> Overview of the implementation:
> When stack walking begins, the StackWalker calls into the VM to anchor a 
> native frame (callStackWalk) that will fetch the first batch of stack frames. 
>  VM will then invoke the doStackWalk method so that the consumer starts 
> getting StackFrame object for each frame.  If all frames in the current batch 
> are traversed, it will ask the VM to fetch the next batch.  The library side 
> is doing the filtering of reflection frames.   For this patch, the VM filters 
> of the hidden frames and also filter out Throwable::init related frames for 
> stack trace.
>
> Ultimately we will move to these built-in logic out from the VM to the 
> library but I’d like to separate them as future works.
>
> This patch also includes the change for Throwable to use StackWalker but it’s 
> disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The 
> VM backtrace is well tuned for performance.  So we separate the switch of 
> Throwable to use StackWalker as a follow-on work:
> JDK-8141239 Throwable should use StackWalker API to capture the backtrace
>
> MemberName initialization is one source of overhead and we propose to keep 
> the VM flag -XX:-MemberNameInStackFrame for the time being for the 
> performance work to continue for JDK-8141239.
>
> Thanks
> Mandy
>

-- 
- DML

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

A revised webrev:
 http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.01

javadoc:
  http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

The main change in this version is mostly in the hotspot change to incorporate 
Coleen’s suggestion:
1. Move the new Klasses to systemDictionary as well known classes.
2. Cache the doStackWalker method
3. Move StackFrameInfo::mid, version, cpref fields as VM injected fields as 
they are not needed by Java.
These fields are interim for performance measurement to compare the cost of 
MemberName initialization.

The microbenchmark running StackWalker::walk shows 3-4.5X slower due to the use 
of MemberName.  I suspect the overhead might be due to the MemberName 
initialization has to handle unresolved and unlinked methods.  For stack 
walker, the method has been linked and it may be a low hanging fruit to fix in 
the VM.

4. Updates the javadoc of StackWalker::getCallerClass to reflect filtering of 
reflection and MethodHandle frames.

Open question:
- is it useful to have an option to configure filtering/showing 
java.lang.invoke frames?   A user can filter it themselves and I don’t think 
it’s highly needed and we can add it in the future if needed.

Mandy


> On Nov 9, 2015, at 6:32 PM, Mandy Chung  wrote:
> 
> javadoc:
>   
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
> 
> webrev:
>  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
> 
> Overview of the implementation:
> When stack walking begins, the StackWalker calls into the VM to anchor a 
> native frame (callStackWalk) that will fetch the first batch of stack frames. 
>  VM will then invoke the doStackWalk method so that the consumer starts 
> getting StackFrame object for each frame.  If all frames in the current batch 
> are traversed, it will ask the VM to fetch the next batch.  The library side 
> is doing the filtering of reflection frames.   For this patch, the VM filters 
> of the hidden frames and also filter out Throwable::init related frames for 
> stack trace.
> 
> Ultimately we will move to these built-in logic out from the VM to the 
> library but I’d like to separate them as future works.
> 
> This patch also includes the change for Throwable to use StackWalker but it’s 
> disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The 
> VM backtrace is well tuned for performance.  So we separate the switch of 
> Throwable to use StackWalker as a follow-on work:
>   JDK-8141239 Throwable should use StackWalker API to capture the backtrace
> 
> MemberName initialization is one source of overhead and we propose to keep 
> the VM flag -XX:-MemberNameInStackFrame for the time being for the 
> performance work to continue for JDK-8141239.  
> 
> Thanks
> Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Daniel Fuchs]

Hi Mandy,

Looking at stackwalk.cpp, I'm puzzled by this comment at line 465:

http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.01/hotspot/src/share/vm/prims/stackwalk.cpp.html

 463   vframeStream& vfst = anchor.vframe_stream();
 464   if (!vfst.at_end()) {
 465 vfst.next();  // skip java.lang.StackStream::moreStackWalk

Is that because the previous call to decode_frames is supposed
to leave the stream at the last recorded frame - without advancing?
So the frame at vfst had already been returned in the previous batch?

If so I find the comment at line 465 a bit misleading.

best regards,

-- daniel

On 13/11/15 16:21, Mandy Chung wrote:

A revised webrev:
  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.01

javadoc:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

The main change in this version is mostly in the hotspot change to incorporate 
Coleen’s suggestion:
1. Move the new Klasses to systemDictionary as well known classes.
2. Cache the doStackWalker method
3. Move StackFrameInfo::mid, version, cpref fields as VM injected fields as 
they are not needed by Java.
These fields are interim for performance measurement to compare the cost of 
MemberName initialization.

The microbenchmark running StackWalker::walk shows 3-4.5X slower due to the use 
of MemberName.  I suspect the overhead might be due to the MemberName 
initialization has to handle unresolved and unlinked methods.  For stack 
walker, the method has been linked and it may be a low hanging fruit to fix in 
the VM.

4. Updates the javadoc of StackWalker::getCallerClass to reflect filtering of 
reflection and MethodHandle frames.

Open question:
- is it useful to have an option to configure filtering/showing 
java.lang.invoke frames?   A user can filter it themselves and I don’t think 
it’s highly needed and we can add it in the future if needed.

Mandy



On Nov 9, 2015, at 6:32 PM, Mandy Chung  wrote:

javadoc:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

webrev:
  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/

Overview of the implementation:
When stack walking begins, the StackWalker calls into the VM to anchor a native 
frame (callStackWalk) that will fetch the first batch of stack frames.  VM will 
then invoke the doStackWalk method so that the consumer starts getting 
StackFrame object for each frame.  If all frames in the current batch are 
traversed, it will ask the VM to fetch the next batch.  The library side is 
doing the filtering of reflection frames.   For this patch, the VM filters of 
the hidden frames and also filter out Throwable::init related frames for stack 
trace.

Ultimately we will move to these built-in logic out from the VM to the library 
but I’d like to separate them as future works.

This patch also includes the change for Throwable to use StackWalker but it’s 
disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The VM 
backtrace is well tuned for performance.  So we separate the switch of 
Throwable to use StackWalker as a follow-on work:
   JDK-8141239 Throwable should use StackWalker API to capture the backtrace

MemberName initialization is one source of overhead and we propose to keep the 
VM flag -XX:-MemberNameInStackFrame for the time being for the performance work 
to continue for JDK-8141239.

Thanks
Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Daniel Fuchs]

Hi Mandy,

StackFrameInfo.java:

 100 public OptionalInt getLineNumber() {
 101 ensureMethodInfoInitialized();
 102 return lineNumber != -1 ? OptionalInt.of(lineNumber) : 
OptionalInt.empty();

 103 }

If lineNumber is -2 then empty should probably be returned too.

Which means that the following code code in StackWalker.StackFrame
would need to be changed accordingly:

 175 public default StackTraceElement toStackTraceElement() {
 176 return new StackTraceElement(getClassName(), 
getMethodName(),

 177  getFileName().orElse(null),
 178  getLineNumber().orElse(-1));
 179 }

The other option would be to document -2 as a special value
in StackFrame.getLineNumber() but that would be ugly?

best regards

-- daniel

-- daniel

On 13/11/15 16:21, Mandy Chung wrote:

A revised webrev:
  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.01

javadoc:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

The main change in this version is mostly in the hotspot change to incorporate 
Coleen’s suggestion:
1. Move the new Klasses to systemDictionary as well known classes.
2. Cache the doStackWalker method
3. Move StackFrameInfo::mid, version, cpref fields as VM injected fields as 
they are not needed by Java.
These fields are interim for performance measurement to compare the cost of 
MemberName initialization.

The microbenchmark running StackWalker::walk shows 3-4.5X slower due to the use 
of MemberName.  I suspect the overhead might be due to the MemberName 
initialization has to handle unresolved and unlinked methods.  For stack 
walker, the method has been linked and it may be a low hanging fruit to fix in 
the VM.

4. Updates the javadoc of StackWalker::getCallerClass to reflect filtering of 
reflection and MethodHandle frames.

Open question:
- is it useful to have an option to configure filtering/showing 
java.lang.invoke frames?   A user can filter it themselves and I don’t think 
it’s highly needed and we can add it in the future if needed.

Mandy



On Nov 9, 2015, at 6:32 PM, Mandy Chung  wrote:

javadoc:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

webrev:
  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/

Overview of the implementation:
When stack walking begins, the StackWalker calls into the VM to anchor a native 
frame (callStackWalk) that will fetch the first batch of stack frames.  VM will 
then invoke the doStackWalk method so that the consumer starts getting 
StackFrame object for each frame.  If all frames in the current batch are 
traversed, it will ask the VM to fetch the next batch.  The library side is 
doing the filtering of reflection frames.   For this patch, the VM filters of 
the hidden frames and also filter out Throwable::init related frames for stack 
trace.

Ultimately we will move to these built-in logic out from the VM to the library 
but I’d like to separate them as future works.

This patch also includes the change for Throwable to use StackWalker but it’s 
disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The VM 
backtrace is well tuned for performance.  So we separate the switch of 
Throwable to use StackWalker as a follow-on work:
   JDK-8141239 Throwable should use StackWalker API to capture the backtrace

MemberName initialization is one source of overhead and we propose to keep the 
VM flag -XX:-MemberNameInStackFrame for the time being for the performance work 
to continue for JDK-8141239.

Thanks
Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 13, 2015, at 9:55 AM, Daniel Fuchs  wrote:
> 
> Hi Mandy,
> 
> StackFrameInfo.java:
> 
> 100 public OptionalInt getLineNumber() {
> 101 ensureMethodInfoInitialized();
> 102 return lineNumber != -1 ? OptionalInt.of(lineNumber) : 
> OptionalInt.empty();
> 103 }
> 
> If lineNumber is -2 then empty should probably be returned too.

lineNumber == -2 is used to indicate a native method.

-1 is unavailable.

> 
> Which means that the following code code in StackWalker.StackFrame
> would need to be changed accordingly:
> 
> 175 public default StackTraceElement toStackTraceElement() {
> 176 return new StackTraceElement(getClassName(), getMethodName(),
> 177  getFileName().orElse(null),
> 178  getLineNumber().orElse(-1));
> 179 }
> 
> The other option would be to document -2 as a special value
> in StackFrame.getLineNumber() but that would be ugly?
> 

StackTraceElement constructor takes -2 line number for native method.

Mandy

> best regards
> 
> -- daniel
> 
> -- daniel
> 
> On 13/11/15 16:21, Mandy Chung wrote:
>> A revised webrev:
>>  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.01
>> 
>> javadoc:
>>   
>> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
>> 
>> The main change in this version is mostly in the hotspot change to 
>> incorporate Coleen’s suggestion:
>> 1. Move the new Klasses to systemDictionary as well known classes.
>> 2. Cache the doStackWalker method
>> 3. Move StackFrameInfo::mid, version, cpref fields as VM injected fields as 
>> they are not needed by Java.
>> These fields are interim for performance measurement to compare the cost of 
>> MemberName initialization.
>> 
>> The microbenchmark running StackWalker::walk shows 3-4.5X slower due to the 
>> use of MemberName.  I suspect the overhead might be due to the MemberName 
>> initialization has to handle unresolved and unlinked methods.  For stack 
>> walker, the method has been linked and it may be a low hanging fruit to fix 
>> in the VM.
>> 
>> 4. Updates the javadoc of StackWalker::getCallerClass to reflect filtering 
>> of reflection and MethodHandle frames.
>> 
>> Open question:
>> - is it useful to have an option to configure filtering/showing 
>> java.lang.invoke frames?   A user can filter it themselves and I don’t think 
>> it’s highly needed and we can add it in the future if needed.
>> 
>> Mandy
>> 
>> 
>>> On Nov 9, 2015, at 6:32 PM, Mandy Chung  wrote:
>>> 
>>> javadoc:
>>>   
>>> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
>>> 
>>> webrev:
>>>  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
>>> 
>>> Overview of the implementation:
>>> When stack walking begins, the StackWalker calls into the VM to anchor a 
>>> native frame (callStackWalk) that will fetch the first batch of stack 
>>> frames.  VM will then invoke the doStackWalk method so that the consumer 
>>> starts getting StackFrame object for each frame.  If all frames in the 
>>> current batch are traversed, it will ask the VM to fetch the next batch.  
>>> The library side is doing the filtering of reflection frames.   For this 
>>> patch, the VM filters of the hidden frames and also filter out 
>>> Throwable::init related frames for stack trace.
>>> 
>>> Ultimately we will move to these built-in logic out from the VM to the 
>>> library but I’d like to separate them as future works.
>>> 
>>> This patch also includes the change for Throwable to use StackWalker but 
>>> it’s disabled by default.  To enable it, set -Dstackwalk.newThrowable=true. 
>>>  The VM backtrace is well tuned for performance.  So we separate the switch 
>>> of Throwable to use StackWalker as a follow-on work:
>>>   JDK-8141239 Throwable should use StackWalker API to capture the backtrace
>>> 
>>> MemberName initialization is one source of overhead and we propose to keep 
>>> the VM flag -XX:-MemberNameInStackFrame for the time being for the 
>>> performance work to continue for JDK-8141239.
>>> 
>>> Thanks
>>> Mandy
>> 
> 

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

The comment was incorrect.  It should be:

// this was the last frame decoded in the previous batch

Mandy


> On Nov 13, 2015, at 9:36 AM, Daniel Fuchs  wrote:
> 
> Hi Mandy,
> 
> Looking at stackwalk.cpp, I'm puzzled by this comment at line 465:
> 
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.01/hotspot/src/share/vm/prims/stackwalk.cpp.html
> 
> 463   vframeStream& vfst = anchor.vframe_stream();
> 464   if (!vfst.at_end()) {
> 465 vfst.next();  // skip java.lang.StackStream::moreStackWalk
> 
> Is that because the previous call to decode_frames is supposed
> to leave the stream at the last recorded frame - without advancing?
> So the frame at vfst had already been returned in the previous batch?
> 
> If so I find the comment at line 465 a bit misleading.
> 
> best regards,
> 
> -- daniel
> 
> On 13/11/15 16:21, Mandy Chung wrote:
>> A revised webrev:
>>  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.01
>> 
>> javadoc:
>>   
>> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
>> 
>> The main change in this version is mostly in the hotspot change to 
>> incorporate Coleen’s suggestion:
>> 1. Move the new Klasses to systemDictionary as well known classes.
>> 2. Cache the doStackWalker method
>> 3. Move StackFrameInfo::mid, version, cpref fields as VM injected fields as 
>> they are not needed by Java.
>> These fields are interim for performance measurement to compare the cost of 
>> MemberName initialization.
>> 
>> The microbenchmark running StackWalker::walk shows 3-4.5X slower due to the 
>> use of MemberName.  I suspect the overhead might be due to the MemberName 
>> initialization has to handle unresolved and unlinked methods.  For stack 
>> walker, the method has been linked and it may be a low hanging fruit to fix 
>> in the VM.
>> 
>> 4. Updates the javadoc of StackWalker::getCallerClass to reflect filtering 
>> of reflection and MethodHandle frames.
>> 
>> Open question:
>> - is it useful to have an option to configure filtering/showing 
>> java.lang.invoke frames?   A user can filter it themselves and I don’t think 
>> it’s highly needed and we can add it in the future if needed.
>> 
>> Mandy
>> 
>> 
>>> On Nov 9, 2015, at 6:32 PM, Mandy Chung  wrote:
>>> 
>>> javadoc:
>>>   
>>> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
>>> 
>>> webrev:
>>>  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
>>> 
>>> Overview of the implementation:
>>> When stack walking begins, the StackWalker calls into the VM to anchor a 
>>> native frame (callStackWalk) that will fetch the first batch of stack 
>>> frames.  VM will then invoke the doStackWalk method so that the consumer 
>>> starts getting StackFrame object for each frame.  If all frames in the 
>>> current batch are traversed, it will ask the VM to fetch the next batch.  
>>> The library side is doing the filtering of reflection frames.   For this 
>>> patch, the VM filters of the hidden frames and also filter out 
>>> Throwable::init related frames for stack trace.
>>> 
>>> Ultimately we will move to these built-in logic out from the VM to the 
>>> library but I’d like to separate them as future works.
>>> 
>>> This patch also includes the change for Throwable to use StackWalker but 
>>> it’s disabled by default.  To enable it, set -Dstackwalk.newThrowable=true. 
>>>  The VM backtrace is well tuned for performance.  So we separate the switch 
>>> of Throwable to use StackWalker as a follow-on work:
>>>   JDK-8141239 Throwable should use StackWalker API to capture the backtrace
>>> 
>>> MemberName initialization is one source of overhead and we propose to keep 
>>> the VM flag -XX:-MemberNameInStackFrame for the time being for the 
>>> performance work to continue for JDK-8141239.
>>> 
>>> Thanks
>>> Mandy
>> 
> 

Re: Code Review for JEP 259: Stack-Walking API

[David M. Lloyd]

One other kind of stack metadata that is missing (yet is present with 
external tools such as jstack etc.) is ancillary data about stack frames 
indicating whether the frame holds or is waiting on a lock.  I'm not 
sure whether it's practical to add all this stuff though, 
performance-wise, unless maybe it is made optional with an option flag.


On 11/12/2015 09:29 AM, Jason Mehrens wrote:

Mandy,

Forgive my ignorance on this subject and impacts but, would it be possible to 
include more metadata methods in the StackWalker.StackFrame interface such as 
getParameterTypes/parameterArray and getReturnType/returnType?  Ideally, I 
really would like these methods for StackTraceElement since knowing the exact 
parameter types allows the caller to lookup the method to examine all the 
method metadata which is useful for logging not only in output but for use in 
stacktrace reduction algorithms.

Thanks,

Jason


From: core-libs-dev  on behalf of Mandy Chung 

Sent: Monday, November 9, 2015 8:32 PM
To: core-libs-dev; hotspot-runtime-...@openjdk.java.net
Subject: Code Review for JEP 259: Stack-Walking API

javadoc:

http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

webrev:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/

Overview of the implementation:
When stack walking begins, the StackWalker calls into the VM to anchor a native 
frame (callStackWalk) that will fetch the first batch of stack frames.  VM will 
then invoke the doStackWalk method so that the consumer starts getting 
StackFrame object for each frame.  If all frames in the current batch are 
traversed, it will ask the VM to fetch the next batch.  The library side is 
doing the filtering of reflection frames.   For this patch, the VM filters of 
the hidden frames and also filter out Throwable::init related frames for stack 
trace.

Ultimately we will move to these built-in logic out from the VM to the library 
but I’d like to separate them as future works.

This patch also includes the change for Throwable to use StackWalker but it’s 
disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The VM 
backtrace is well tuned for performance.  So we separate the switch of 
Throwable to use StackWalker as a follow-on work:
JDK-8141239 Throwable should use StackWalker API to capture the backtrace

MemberName initialization is one source of overhead and we propose to keep the 
VM flag -XX:-MemberNameInStackFrame for the time being for the performance work 
to continue for JDK-8141239.

Thanks
Mandy



--
- DML

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

You might want to get back Method (or something equivalent non-executable 
version?) this requires a lot more consideration.  StackWalker opens up this 
possibility for the future.   I’d like to keep the scope for this JEP to do 
stack walking and provide the replacement for 
sun.reflect.Reflection.getCallerClass in JDK 9.

You can file RFE for future consideration.

Mandy

> On Nov 12, 2015, at 7:29 AM, Jason Mehrens  wrote:
> 
> Mandy,
> 
> Forgive my ignorance on this subject and impacts but, would it be possible to 
> include more metadata methods in the StackWalker.StackFrame interface such as 
> getParameterTypes/parameterArray and getReturnType/returnType?  Ideally, I 
> really would like these methods for StackTraceElement since knowing the exact 
> parameter types allows the caller to lookup the method to examine all the 
> method metadata which is useful for logging not only in output but for use in 
> stacktrace reduction algorithms.
> 
> Thanks,
> 
> Jason
> 
> 
> From: core-libs-dev  on behalf of 
> Mandy Chung 
> Sent: Monday, November 9, 2015 8:32 PM
> To: core-libs-dev; hotspot-runtime-...@openjdk.java.net
> Subject: Code Review for JEP 259: Stack-Walking API
> 
> javadoc:
>   
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
> 
> webrev:
>  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
> 
> Overview of the implementation:
> When stack walking begins, the StackWalker calls into the VM to anchor a 
> native frame (callStackWalk) that will fetch the first batch of stack frames. 
>  VM will then invoke the doStackWalk method so that the consumer starts 
> getting StackFrame object for each frame.  If all frames in the current batch 
> are traversed, it will ask the VM to fetch the next batch.  The library side 
> is doing the filtering of reflection frames.   For this patch, the VM filters 
> of the hidden frames and also filter out Throwable::init related frames for 
> stack trace.
> 
> Ultimately we will move to these built-in logic out from the VM to the 
> library but I’d like to separate them as future works.
> 
> This patch also includes the change for Throwable to use StackWalker but it’s 
> disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The 
> VM backtrace is well tuned for performance.  So we separate the switch of 
> Throwable to use StackWalker as a follow-on work:
>   JDK-8141239 Throwable should use StackWalker API to capture the backtrace
> 
> MemberName initialization is one source of overhead and we propose to keep 
> the VM flag -XX:-MemberNameInStackFrame for the time being for the 
> performance work to continue for JDK-8141239.
> 
> Thanks
> Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 10, 2015, at 3:05 AM, Jaroslav Bachorik  
> wrote:
> 
> Mostly nits and typos.
> 
> hotspot/src/share/vm/classfile/javaClasses.cpp
> L1941 - variable 'cpref' is not used
> 

Fixed.

> hotspot/src/share/vm/classfile/javaClasses.inline.hpp
> L117 - line number 'bci + 100' has a special meaning?

This is existing code refactored from javaClasses.hpp.  I don’t know why it 
added  +100.  For hidden frames, keeping line number to -1 is good to me 
since it means unavailable.

The runtime team may have the answer.

> 
> hotspot/src/share/vm/prims/stackwalk.cpp
> L471 - typo 'pendiing' -> ‘pending'
> 

Fixed.

> src/java.base/share/classes/java/lang/LiveStackFrameInfo.java
> L40,L68 - typo '""liveStackFrames"' -> '"liveStackFrames"'
> L65-66 - probably a redundant info (the exact permission is listed on L67-68
> 

Fixed and the getStackWalker methods are moved to LiveStackFrame.java. 

> jdk/src/java.base/share/classes/java/lang/StackWalker.java
> L222 - the documentation states only locals and operands while also monitors 
> are to be collected
> 

Fixed.

> jdk/src/java.base/share/classes/java/lang/StackStreamFactory.java
> L478-481 - should 'initialBatchSize' be >= START_POS ?

It should be >= MIN_BATCH_SIZE.  I add a check there.

> L574 - 'throw new IllegalStateException("origin " + origin + " != " + 
> skipFrames);' -> 'throw new IllegalStateException("origin " + origin + " != " 
> + (skipFrames + START_POS));’
> 
Fixed.

> jdk/test/java/lang/StackWalker/VerifyStackTrace.java
> L66,96,130 - 'the you can' -> 'you can’
> 

Fixed.

> There are several '// TODO' comments in the sources and tests - is there a 
> plan to address them?
> 

Yes.  The tests are being cleaned up in the next revision.

Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 10, 2015, at 6:05 AM, Dmitry Samersoff  
> wrote:
> 
> Mandy,
> 
> Native part looks good for me.
> 
> javaClasses.cpp:
> 
> 1. It might be good to create a helper inline function for
> 
>  int bci_version = stackFrame->int_field(bci_offset);
>  int version = BackTrace::version_at(bci_version);
> 

Coleen gave me some suggestion to use injected fields.  This will be changed in 
the next revision.

> 2. java_lang_StackFrameInfo::fill_methodInfo
> 
>  CHECK macro left methodInfo partially initialized, not sure it's OK.
> 

Any exception here will cause StackWalker::walk to fail.  I expect the 
exception thrown here  would be OOME and other internal errors.  So it should 
be okay.

> javaClasses.inline.hpp:
> 
> 78 You can use the same pattern as in assert:
> 
>if ((jushort)version != version) version = USHRT_MAX;
> 
> 117 Is it possible to add comment about +100 magic?
> 

This is existing code refactored from javaClasses.hpp.  I don’t know why it 
added  +100.  For hidden frames, keeping line number to -1 is good to me 
since it means unavailable.

Coleen - do you know why this is done this way?

> jvm.cpp:
> 
> 627 missed space around =

Fixed.
Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Dmitry Samersoff]

Mandy,

Native part looks good for me.

javaClasses.cpp:

1. It might be good to create a helper inline function for

  int bci_version = stackFrame->int_field(bci_offset);
  int version = BackTrace::version_at(bci_version);

2. java_lang_StackFrameInfo::fill_methodInfo

  CHECK macro left methodInfo partially initialized, not sure it's OK.

javaClasses.inline.hpp:

78 You can use the same pattern as in assert:

if ((jushort)version != version) version = USHRT_MAX;

117 Is it possible to add comment about +100 magic?

jvm.cpp:

627 missed space around =

-Dmitry


On 2015-11-10 05:32, Mandy Chung wrote:
> javadoc:
>
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
> 
> webrev:
>   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
> 
> Overview of the implementation:
> When stack walking begins, the StackWalker calls into the VM to anchor a 
> native frame (callStackWalk) that will fetch the first batch of stack frames. 
>  VM will then invoke the doStackWalk method so that the consumer starts 
> getting StackFrame object for each frame.  If all frames in the current batch 
> are traversed, it will ask the VM to fetch the next batch.  The library side 
> is doing the filtering of reflection frames.   For this patch, the VM filters 
> of the hidden frames and also filter out Throwable::init related frames for 
> stack trace.
> 
> Ultimately we will move to these built-in logic out from the VM to the 
> library but I’d like to separate them as future works.
> 
> This patch also includes the change for Throwable to use StackWalker but it’s 
> disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The 
> VM backtrace is well tuned for performance.  So we separate the switch of 
> Throwable to use StackWalker as a follow-on work:
>JDK-8141239 Throwable should use StackWalker API to capture the backtrace
> 
> MemberName initialization is one source of overhead and we propose to keep 
> the VM flag -XX:-MemberNameInStackFrame for the time being for the 
> performance work to continue for JDK-8141239.  
> 
> Thanks
> Mandy
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.