Code Review for JEP 259: Stack-Walking API (original) (raw)

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri Nov 20 11:33:51 UTC 2015

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.spitsyn at 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* javalangStackFrameInfo::getfilename(Handle stackFrame, InstanceKlass* holder) { 2129 if (MemberNameInStackFrame) { 2130 return holder->sourcefilename(); 2131 } else { 2132 int bci = stackFrame->intfield(bcioffset); 2133 short version = stackFrame->shortfield(versionoffset); 2134 2135 return Backtrace::getsourcefilename(holder, version); 2136 } 2137 } The 'int bci' is not used above. 2139 void javalangStackFrameInfo::setmethodandbci(Handle stackFrame, const methodHandle& method, int bci) { 2140 if (MemberNameInStackFrame) { 2141 oop mname = stackFrame->objfield(memberNameoffset); 2142 InstanceKlass* ik = method->methodholder(); 2143 CallInfo info(method(), ik); 2144 MethodHandles::initmethodMemberName(mname, info); 2145 javalangStackFrameInfo::setbci(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 javalangStackFrameInfo::setversion(stackFrame(), (short)version); 2150 } else { 2151 int mid = method->origmethodidnum(); 2152 int version = method->constants()->version(); 2153 int cpref = method->nameindex(); 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 javalangStackFrameInfo::setmid(stackFrame(), (short)mid); 2159 javalangStackFrameInfo::setversion(stackFrame(), (short)version); 2160 javalangStackFrameInfo::setcpref(stackFrame(), (short)cpref); 2161 javalangStackFrameInfo::setbci(stackFrame(), bci); 2162 } 2163 } Minor: The calls to setversion() and setbci() 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 javalangStackFrameInfo::setmethodandbci(Handle stackFrame, const methodHandle& method, int bci) { javalangStackFrameInfo::setbci(stackFrame(), bci); // method may be redefined; store the version int version = method->constants()->version(); assert((jushort)version == version, "version should be short"); javalangStackFrameInfo::setversion(stackFrame(), (short)version); if (MemberNameInStackFrame) { oop mname = stackFrame->objfield(memberNameoffset); InstanceKlass* ik = method->methodholder(); CallInfo info(method(), ik); MethodHandles::initmethodMemberName(mname, info); } else { int mid = method->origmethodidnum(); int cpref = method->nameindex(); assert((jushort)mid == mid, "mid should be short"); assert((jushort)cpref == cpref, "cpref should be short"); javalangStackFrameInfo::setmid(stackFrame(), (short)mid); javalangStackFrameInfo::setcpref(stackFrame(), (short)cpref); } } webrev.03/hotspot/src/share/vm/prims/jvm.cpp 572 int limit = startindex+framecount; 597 int limit = startindex+framecount; 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 // classesarray User-supplied buffers. The 0th element is reserved . . . 86 // Parameters: 87 // mode. Restrict which frames to be decoded. 88 // decodelimit. Maximum of frames to be decoded. 89 // startindex. Start index to the user-supplied buffers. 90 // classesarray. Buffer to store classes in, starting at startindex. 91 // framesarray. Buffer to store StackFrame in, starting at startindex. 92 // NULL if not used. 93 // endindex. End index to the user-supplied buffers with unpacked frames. 94 // . . . 274 // Parameters: 275 // stackStream. StackStream object 276 // mode. Stack walking mode. 277 // skipframes. Number of frames to be skipped. 278 // framecount. Number of frames to be traversed. 279 // startindex. Start index to the user-supplied buffers. 280 // classesarray. Buffer to store classes in, starting at startindex. 281 // framesarray. Buffer to store StackFrame in, starting at startindex. . . . 414 // Parameters: 415 // stackStream. StackStream object 416 // mode. Stack walking mode. 417 // magic. Must be valid value to continue the stack walk 418 // framecount. Number of frames to be decoded. 419 // startindex. Start index to the user-supplied buffers. 420 // classesarray. Buffer to store classes in, starting at startindex. 421 // framesarray. Buffer to store StackFrame in, starting at startindex. Minor: Dots after the parameter names looks strange. Probably better to remove them or replace with colons. Also, the line 65 is inconsistent with others. 114 if (!ShowHiddenFrames && StackWalk::skiphiddenframes(mode)) { 115 if (method->ishidden()) { 116 if (TraceStackWalk) { 117 tty->print(" hidden method: "); method->printshortname(); 118 tty->print("\n"); 119 } 120 continue; 121 } 122 } Minor: Indent at the line 115 must be +2. 338 if (!skiptofillInStackTrace) { 339 if (ik == SystemDictionary::Throwableklass() && 340 method->name() == vmSymbols::fillInStackTracename()) { 341 // this frame will be skipped 342 skiptofillInStackTrace = true; 343 } 344 } else if (!(ik->issubclassof(SystemDictionary::Throwableklass()) && 345 method->name() == vmSymbols::objectinitializername())) { 346 // there are none or we've seen them all - either way stop checking 347 skipthrowableInitcheck = true; 348 break; 349 } Minor: Indent must be +2 at 341 and 347. 360 for (int n=0; n < skipframes && !vfst.atend(); vfst.next(), n++) { 451 int count = framecount+startindex; Minor: Need spaces around the '=' and '+'. 392 // Link the thread and vframe stream into the callee-visible object: 397 // Do this before anything else happens, to disable any lingering stream objects: 400 // Throw pending exception if we must: Minor: Better to place dots instead of colons at the end. Thanks, Serguei

On 11/19/15 18: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. Mandy

On Nov 19, 2015, at 1:38 PM, Coleen Phillimore <coleen.phillimore at oracle.com> 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

More information about the hotspot-runtime-dev mailing list