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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri Nov 20 09:59:41 UTC 2015

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 replace with colons.
Also, the line 65 is inconsistent with others.

114 if (!ShowHiddenFrames && StackWalk::skip_hidden_frames(mode)) { 115 if (method->is_hidden()) { 116 if (TraceStackWalk) { 117 tty->print(" hidden method: "); method->print_short_name(); 118 tty->print("\n"); 119 } 120 continue; 121 } 122 }

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

338 if (!skip_to_fillInStackTrace) { 339 if (ik == SystemDictionary::Throwable_klass() && 340 method->name() == vmSymbols::fillInStackTrace_name()) { 341 // this frame will be skipped 342 skip_to_fillInStackTrace = true; 343 } 344 } else if (!(ik->is_subclass_of(SystemDictionary::Throwable_klass()) && 345 method->name() == vmSymbols::object_initializer_name())) { 346 // there are none or we've seen them all - either way stop checking 347 skip_throwableInit_check = true; 348 break; 349 }

Minor: Indent must be +2 at 341 and 347.

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

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