Request for review (#4) 7174978: NPG: Fix bactrace builder for class redefinition
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Jan 17 11:25:38 PST 2013
It looks good.
Thank you for the nit fixes!
Thanks,
Serguei
On 1/17/13 10:49 AM, Coleen Phillimore wrote:
>
> 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/60202c4a/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list