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

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.

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

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

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

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.

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

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:

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

> 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

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

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

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

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

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

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,

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

> 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

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

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.

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

> 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: > >

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

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.

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

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

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

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

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

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

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

> 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

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

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

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

> 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

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

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

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

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

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

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)

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

, 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

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

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

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

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

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

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

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

> 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) : >

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

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: > >

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

view 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 anc

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

> 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/jav

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

> 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

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

> 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

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

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