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