Request for review (#4) 7174978: NPG: Fix bactrace builder for class redefinition

Coleen Phillimore coleen.phillimore at
Thu Jan 17 10:49:23 PST 2013

On 01/17/2013 12:50 PM, serguei.spitsyn at wrote:
> Coleen,
> Looked at this version of webrev:
> **
> The changes look good, great work!
> Sorry for the latency.
> I've a couple of nits and one question, all about this file:
> *  src/share/vm/classfile/javaClasses.cpp*
> Ii is Ok to skip my nits though.
> *Nit 1:*
> Three functions (*get_methods**, get_bcis, get_mirrors*) have the same
> assert message *"sanity check"*:
> 1309   // get info out of chunks
> 1310   static typeArrayOop*get_methods*(objArrayHandle chunk) {
> 1311     typeArrayOop methods = typeArrayOop(chunk->obj_at(trace_methods_offset));
> 1312     assert(methods != NULL,*"sanity check"*);
> 1313     return methods;
> 1314   }
> It is more helpful if the messages are more specific.

Okay, I also don't like "sanity check" as an assertion message. I'll say 
"methods|bcis|mirror array should be initialized in backtrace"
> *Nit 2:*
> It'd simplify code a little bit it the second of the two 
> *print_stack_element*() functions
> makes a call to the first one instead of the 
> *print_stack_element_to_buffer*.
> It'd save just two lines of code: *ResourceMark* and *print_cr* call.
> 1383 void java_lang_Throwable::*print_stack_element*(outputStream *st, Handle mirror,
> 1384                                               int method_id, int version, int bci) {
> 1385   ResourceMark rm;
> 1386   char* buf = print_stack_element_to_buffer(mirror, method_id, version, bci);
> 1387   st->print_cr("%s", buf);
> 1388 }
> 1389
> 1390 void java_lang_Throwable::*print_stack_element*(outputStream *st, methodHandle method, int bci) {
> 1391   ResourceMark rm;
> 1392   Handle mirror = method->method_holder()->java_mirror();
> 1393   int method_id = method->method_idnum();
> 1394   int version = method->constants()->version();
> 1395   char* buf = print_stack_element_to_buffer(mirror, method_id, version, bci);
> 1396   st->print_cr("%s", buf);
> 1397 }

Yes, that's good.  I did that.
> *Nit 3:*
> It seems as possible to rearrange the code a little bit so that the 
> constructor
> *BacktraceBuilder**(ObjArrayOop backtrace)* could call the functions
> *get_methods()*, *get_bcis()* and *get_mirror()* instead of doing the 
> same on its own.
> The functions will need to accept *Oop* arguments instead of *Handle*'s.

Thanks!  That's a nice suggestion.   I made the 
BacktraceBuilder(objArrayHandle) argument a handle and took out some 
oop->handle conversions in the caller.

> *Question 1**:*
> Is it Ok that some of the *BacktraceBuilder* fields are *Oops*, not 
> *Handles*?

Yes, it's disturbing but there is a No_Safepoint_Verifier in 
BacktraceBuilder so it asserts that you won't hit a safepoint and can 
hold oops in it.   Someone must have decided this was more performant.

I made your suggested changes and the new webrev is at

Thank you for the review and improvements.


> Thanks,
> Serguei
> On 1/14/13 6:36 PM, Coleen Phillimore wrote:
>> Summary: Remove Method* from backtrace but save version so redefine 
>> classes doesn't give inaccurate line numbers.  Removed old Merlin API 
>> with duplicate code.
>> Sorry, this is so long but I want to explain this change properly.
>> When exceptions are thrown, the VM saves a backtrace for each which 
>> may or may not be printed or saved for later.   For speed, the 
>> information that the VM saves are arrays 32 of Method* and bci. For 
>> permgen elimination we added an array of mirrors (instances of 
>> java_lang_Class), so that the class loader owning the Method* pointer 
>> doesn't get unloaded and deallocated.
>> The problem with redefine classes is that the method can be redefined 
>> and the Method* pointer can be deallocated even if the class loader 
>> owning the Method* is kept alive.    Printing or touching the 
>> backtrace later will crash the VM.
>> This is my 4th attempt at fixing this problem.   The first that was 
>> reviewed, saved a side structure for each backtrace created so that 
>> we can walk the Method* pointers and keep them from being 
>> deallocated.   This change had an unfortunate side structure to 
>> manage, and had also a 6.1% slowdown in javac. The second attempt was 
>> to predigest the Method* into the data needed for 
>> java_lang_StackTraceElement into method_name, class_name, file_name, 
>> and line_number, so that the Method* doesn't need to be saved. This 
>> had a 18% slowdown in javac.
>> The 3rd attempt is to look for a way to have GC handle backtraces and 
>> mark methods as on_stack but adding a InstanceThrowableKlass and 
>> determining which closures needed special actions for the array of 
>> Method* pointers is a lot of additional special case code to the 
>> garbage collectors.  It's unclear how this would work actually, but 
>> we might have to revisit this idea for java_lang_invoke_MemberName.
>> The 4th attempt is to save the method_idnum, bci, version_number, and 
>> mirror.   From the method_idnum, we can get the Method*.   The 
>> version number is updated for redefine classes.   There was an unused 
>> field that I repurposed.   If the method's version doesn't match the 
>> backtrace, we do not return the source_file and line_number from the 
>> Method* because it's changed.   You get a backtrace of the form:
>> java.lang.RuntimeException: Test exception
>>         at RedefineMethodInBacktraceTarget.methodToRedefine(Unknown 
>> Source)
>>         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>>         at 
>> sun.reflect.NativeMethodAccessorImpl.invoke(
>>         at 
>> sun.reflect.DelegatingMethodAccessorImpl.invoke(
>>         at java.lang.reflect.Method.invoke(
>>         at 
>> RedefineMethodInBacktraceApp.getThrowableFromMethodToRedefine(
>>         at 
>> RedefineMethodInBacktraceApp.doMethodInBacktraceTest(
>> This fix for the Method* crash does not add to the footprint of the 
>> backtraces (idnum is short and bci and version number is packed into 
>> an int).  It does not degrade performance of javac (or 
>> refworkload).   The cost is to not support getting a source file and 
>> line number from a method that was redefined after being saved in a 
>> backtrace.  Note that this change does not return the wrong 
>> information.   A CR can be filed for somebody to someday lift this 
>> restriction if someday we don't care about performance or a new 
>> solution is found.
>> This might be hard to review as a diff because I refactored 
>> duplicated code.
>> The webrev:
>> Bug link:
>> Tested with NSK quick testlist, runThese jck tests, JPRT, refworkload.
>> Thanks,
>> Coleen

-------------- next part --------------
An HTML attachment was scrubbed...

More information about the hotspot-runtime-dev mailing list