Request for review (#4) 7174978: NPG: Fix bactrace builder for class redefinition
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Jan 17 10:49:23 PST 2013
On 01/17/2013 12:50 PM, serguei.spitsyn at oracle.com wrote:
> 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.
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
http://cr.openjdk.java.net/~coleenp/7174978_7
Thank you for the review and improvements.
Coleen
>
>
> 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/5a1580f1/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list