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

[serguei.spitsyn at oracle.com]

Coleen,


Looked at this version of webrev:
*  http://cr.openjdk.java.net/~coleenp/7174978_6/*


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.

*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 }


*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.

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



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(NativeMethodAccessorImpl.java:57)
>         at 
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>         at java.lang.reflect.Method.invoke(Method.java:474)
>         at 
> RedefineMethodInBacktraceApp.getThrowableFromMethodToRedefine(RedefineMethodInBacktraceApp.java:60)
>         at 
> RedefineMethodInBacktraceApp.doMethodInBacktraceTest(RedefineMethodInBacktraceApp.java:44)
>
> 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:  http://cr.openjdk.java.net/~coleenp/7174978_5/
> Bug link: http://bugs.sun.com/view_bug.do?bug_id=7174978
>
> Tested with NSK quick testlist, runThese jck tests, JPRT, refworkload.
>
> Thanks,
> Coleen
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130117/064cbbc7/attachment.html