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

2015-11-20 Thread 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 A

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

2015-11-20 Thread 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/w

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

2015-11-20 Thread 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/class

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

2015-11-20 Thread 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/s

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

2015-11-20 Thread 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 (MemberNameInS

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

2015-11-19 Thread 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

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

2015-11-18 Thread 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.

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

2015-11-18 Thread 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 strin

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

2015-11-18 Thread Mandy Chung
I want to make sure the key point is getting across. This change does not change anything in Throwable, backtrace and StackTraceElement. StackFrameInfo is used by the new StackWalker API. JDK Thread::dumpStack, Thread::getStackTrace and LogRecord/PlatformLogger are updated to use the stack wa

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

2015-11-18 Thread 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 digeste

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

2015-11-18 Thread 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" lin

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

2015-11-18 Thread 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

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

2015-11-18 Thread 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 o

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

2015-11-17 Thread 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 Pete

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

2015-11-17 Thread 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

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

2015-11-17 Thread 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

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

2015-11-17 Thread 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 p

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

2015-11-17 Thread 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 c

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

2015-11-16 Thread 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

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

2015-11-16 Thread 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 getCallerCl

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

2015-11-16 Thread 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 && l

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

2015-11-16 Thread 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)

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

2015-11-13 Thread 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

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

2015-11-13 Thread 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 } > >

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

2015-11-13 Thread 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 m

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

2015-11-13 Thread 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 jav

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

2015-11-13 Thread 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 syst

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

2015-11-13 Thread Timo Kinnunen
, 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

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

2015-11-12 Thread David M. Lloyd
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.openjd

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

2015-11-12 Thread Mandy Chung
-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/

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

2015-11-11 Thread 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

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

2015-11-11 Thread 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

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

2015-11-10 Thread 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 meth

Code Review for JEP 259: Stack-Walking API

2015-11-09 Thread Mandy Chung
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