Code Review for JEP 259: Stack-Walking API
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* 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