RFR: 6830717: replay of compilations would help with debugging
Christian Thalinger
christian.thalinger at oracle.com
Thu Nov 8 11:31:11 PST 2012
On Nov 6, 2012, at 12:41 PM, yumin.qi at oracle.com wrote:
> Forget to cc to open list.
>
> Thanks
> Yumin
>
> -------- Original Message --------
> Subject: Re: RFR: 6830717: replay of compilations would help with debugging
> Date: Tue, 06 Nov 2012 10:53:26 -0800
> From: yumin.qi at oracle.com
> Organization: Oracle Corporation
> To: serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com>
> CC: Christian Thalinger <christian.thalinger at oracle.com>, Vladimir Kozlov <vladimir.kozlov at oracle.com>, Coleen Phillimore <coleen.phillimore at oracle.com>
>
> Hi, Serguei and all
>
> Made changes according to Serguei's suggestion, but could not change ci<class>.dumpReplayData forward call to <class>.dumpReplayData since there is some data is not consistent in compiler interface after runtime.
> Also changes to move dumpReplayData up to base class (not VMObject), so can save changes to subclasses.
>
> http://cr.openjdk.java.net/~minqi/6830717
src/share/vm/utilities/array.hpp:
! T at(int i) const { assert(i >= 0 && i< _length, "Array out of range"); return _data[i]; }
Could you change the asserts to:
! T at(int i) const { assert(i >= 0 && i< _length, err_msg_res("oob: 0 <= %d < %d", i, _length)); return _data[i]; }
src/share/vm/utilities/vmError.cpp:
+ static bool skip_Replay = false;
We try to use lowercase variable names:
+ static bool skip_replay = false;
Otherwise this looks good.
-- Chris
>
> Please have a look and comment.
>
> Thanks
> Yumin
>
>
>
> On 10/26/2012 3:57 PM, serguei.spitsyn at oracle.com wrote:
>> Continue for C++ files (VM code) ...
>>
>> src/share/vm/utilities/utf8.cpp
>> 165 // returns the quoted ascii length of a 0-terminated utf8 string
>> 166 int UTF8::quoted_ascii_length(const char* utf8_str, int utf8_length) {
>> 167 const char *ptr = (const char *)utf8_str;
>>
>> . . .
>>
>> 183 void UTF8::as_quoted_ascii(const char* utf8_str, char* buf, int buflen) {
>> 184 const char *ptr = (const char *)utf8_str;
>>
>> . . .
>>
>> 203 const char* UTF8::from_quoted_ascii(const char* quoted_ascii_str) {
>> 204 const char *ptr = (const char *)quoted_ascii_str;
>> No need to cast to (const char*) as arguments are already const char*.
>>
>>
>> src/share/vm/ci/ciReplay.cpp
>> 603 if (field_signature[1] == 'B') {
>> 604 value = oopFactory::new_byteArray(length, CHECK);
>> 605 } else if (strcmp(field_signature, "[Z") == 0) {
>> 606 value = oopFactory::new_boolArray(length, CHECK);
>> 607 } else if (strcmp(field_signature, "[C") == 0) {
>> 608 value = oopFactory::new_charArray(length, CHECK);
>> 609 } else if (strcmp(field_signature, "[S") == 0) {
>> 610 value = oopFactory::new_shortArray(length, CHECK);
>> 611 } else if (strcmp(field_signature, "[F") == 0) {
>> 612 value = oopFactory::new_singleArray(length, CHECK);
>> 613 } else if (strcmp(field_signature, "[D") == 0) {
>> 614 value = oopFactory::new_doubleArray(length, CHECK);
>> 615 } else if (strcmp(field_signature, "[I") == 0) {
>> 616 value = oopFactory::new_intArray(length, CHECK);
>> 617 } else if (strcmp(field_signature, "[J") == 0) {
>> 618 value = oopFactory::new_longArray(length, CHECK);
>> 619 } else if (field_signature[0] == '[' && field_signature[1] == 'L') {
>> 620 KlassHandle kelem = resolve_klass(field_signature + 1, CHECK);
>> 621 value = oopFactory::new_objArray(kelem(), length, CHECK);
>> 622 } else {
>> 623 report_error("unhandled array staticfield");
>> 624 }
>> 625 }
>>
>> As Christian already noted this can be rewritten like this:
>>
>> switch (field_signature[1]) {
>> case 'B': value = oopFactory::new_byteArray(length, CHECK);
>> case 'Z': value = oopFactory::new_boolArray(length, CHECK);
>> case 'C': value = oopFactory::new_charArray(length, CHECK);
>> case 'S': value = oopFactory::new_shortArray(length, CHECK);
>> case 'F': value = oopFactory::new_singleArray(length, CHECK);
>> case 'D': value = oopFactory::new_doubleArray(length, CHECK);
>> case 'I': value = oopFactory::new_intArray(length, CHECK);
>> case 'J': value = oopFactory::new_longArray(length, CHECK);
>> case 'L':
>> KlassHandle kelem = resolve_klass(field_signature + 1, CHECK);
>> value = oopFactory::new_objArray(kelem(), length, CHECK);
>> default:
>> report_error("unhandled array staticfield");
>> }
>>
>> Almost the same comment about the lines 629-667.
>>
>>
>> These are the only comments I have.
>>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 10/26/12 12:37 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Yumin,
>>>
>>> agent/src/share/classes/sun/jvm/hotspot/CommandProcessor.java
>>>
>>> 778 // This is used to dump replay data from ciInstanceKlass, ciMehtodData etc
>>> Typo: ciMehtodData =>ciMethodData
>>>
>>> 777 new Command("dumpreplaydata", "dumpreplaydata { <address > | -a | id }", false) {
>>> What is id? Is it a thread name? If so, would it better to say: thread_name or thread_id?
>>> Also, do we need angle brackets: { <address > | -a | <thread_name> } ?
>>>
>>> agent/src/share/classes/sun/jvm/hotspot/ci/ciInstanceKlass.java
>>>
>>> 137 sun.jvm.hotspot.oops.OopField bf = (OopField)f;
>>> The sun.jvm.hotspot.oops package os imported, so the short type name OopField can be used.
>>> No conflicts are expected, right? Otherwise, the "(OopField)f" would not be correct.
>>>
>>> agent/src/share/classes/sun/jvm/hotspot/ci/ciMethod.java
>>>
>>> 97 OopUtilities.escapeString(method.getName().asString()) + " " + method.getSignature().asString() + " " +
>>> 98 method.getInvocationCounter() + " " +
>>> 99 method.getBackedgeCounter() + " " +
>>> 100 interpreterInvocationCount() + " " +
>>> 101 interpreterThrowoutCount() + " " +
>>>
>>> It is better to break down the line 97 into two lines.
>>> It would be shorter and more consistent with the lines 98-101.
>>>
>>>
>>> agent/src/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
>>>
>>> 1055 sun.jvm.hotspot.oops.OopField bf = (OopField)f;
>>> The same as for ciInstanceKlass.java above.
>>>
>>> Could you introduce an utility method printFieldValue(Field f), so that the same code is shared with ciInstanceKlass.java?
>>> It feels that the code is the same.
>>>
>>>
>>> agent/src/share/classes/sun/jvm/hotspot/oops/Method.java
>>>
>>> 368 OopUtilities.escapeString(getName().asString()) + " " + getSignature().asString() + " " +
>>> The same as for the ciMethod.java above.
>>>
>>>
>>> agent/src/share/classes/sun/jvm/hotspot/oops/MethodData.java
>>>
>>> It looks like this new code can be shared with ciMethodData.java.
>>> You also may want to look at other files if similat sharing approach can be used.
>>> We can compare all ciXXX.java with XXX.java.
>>>
>>>
>>> agent/doc/clhsdb.html
>>>
>>> 52 dumpreplaydata <address> | -a | id [>replay.txt] <font color="red">dump replay data into a file</font>
>>> The same comment as for the CommandProcessor.java.
>>>
>>>
>>> agent/doc/c2replay.html
>>>
>>> 17 clhsdb>dumpreplaydata <address> | -a | id > replay.txt
>>> The same comment as for the CommandProcessor.java.
>>>
>>>
>>> I've reviewed only java files so far and decided to provide my feedback early.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 10/26/12 9:33 AM, Yumin Qi wrote:
>>>> Hi,
>>>>
>>>> I need your help to have a second look at the changes based on meta changes --- if OK will do push.
>>>>
>>>> Thanks
>>>> Yumin
>>>>
>>>> On 8/13/2012 5:02 PM, Christian Thalinger wrote:
>>>>> On Aug 6, 2012, at 2:40 PM, yumin.qi at oracle.com wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please give your comments of the changes about
>>>>>> 6830717: replay of compilations would help with debugging.
>>>>>> http://monaco.sfbay.sun.com/detail.jsf?cr=6830717
>>>>>>
>>>>>> Sometime jvm crashes in the process of compilation or in compiled method. To reproduce the compilation process in debug JVM is helpful for quickly identifying root cause.
>>>>>> To do recompilation, we collect data by using of SA (Serviceability Agent) to extract application and system class data from core file. Those information includes nmethod, methodOop, methodDataOop, instanceKlass and corresponding ci counterparts.
>>>>>> With reconfiguring similar (not exactly same) compiling environment as in the core file, try to compile again the failed java methods.
>>>>>>
>>>>>> contributed by Tom R (never). webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~minqi/6830717/<http://cr.openjdk.java.net/%7Eminqi/6830717/>
>>>>> src/share/vm/ci/ciReplay.cpp:
>>>>>
>>>>> 603 if (strcmp(field_signature, "[B") == 0) {
>>>>> 604 oop value = oopFactory::new_byteArray(length, CHECK);
>>>>> 605 java_mirror->obj_field_put(fd.offset(), value);
>>>>> 606 } else if (strcmp(field_signature, "[Z") == 0) {
>>>>> 607 oop value = oopFactory::new_boolArray(length, CHECK);
>>>>> 608 java_mirror->obj_field_put(fd.offset(), value);
>>>>> 609 } else if (strcmp(field_signature, "[C") == 0) {
>>>>> 610 oop value = oopFactory::new_charArray(length, CHECK);
>>>>> 611 java_mirror->obj_field_put(fd.offset(), value);
>>>>> 612 } else if (strcmp(field_signature, "[S") == 0) {
>>>>> 613 oop value = oopFactory::new_shortArray(length, CHECK);
>>>>> 614 java_mirror->obj_field_put(fd.offset(), value);
>>>>> 615 } else if (strcmp(field_signature, "[F") == 0) {
>>>>> 616 oop value = oopFactory::new_singleArray(length, CHECK);
>>>>> 617 java_mirror->obj_field_put(fd.offset(), value);
>>>>> 618 } else if (strcmp(field_signature, "[D") == 0) {
>>>>> 619 oop value = oopFactory::new_doubleArray(length, CHECK);
>>>>> 620 java_mirror->obj_field_put(fd.offset(), value);
>>>>> 621 } else if (strcmp(field_signature, "[I") == 0) {
>>>>> 622 oop value = oopFactory::new_intArray(length, CHECK);
>>>>> 623 java_mirror->obj_field_put(fd.offset(), value);
>>>>> 624 } else if (strcmp(field_signature, "[J") == 0) {
>>>>> 625 oop value = oopFactory::new_longArray(length, CHECK);
>>>>> 626 java_mirror->obj_field_put(fd.offset(), value);
>>>>> 627 } else if (field_signature[0] == '['&& field_signature[1] == 'L') {
>>>>>
>>>>> Can't we switch on the second character? And move this call:
>>>>>
>>>>> 630 java_mirror->obj_field_put(fd.offset(), value);
>>>>>
>>>>> out of the switch?
>>>>>
>>>>> 637 if (strcmp(field_signature, "I") == 0) {
>>>>> 638 int value = atoi(string_value);
>>>>> 639 java_mirror->int_field_put(fd.offset(), value);
>>>>> 640 } else if (strcmp(field_signature, "B") == 0) {
>>>>> 641 int value = atoi(string_value);
>>>>> 642 java_mirror->byte_field_put(fd.offset(), value);
>>>>> 643 } else if (strcmp(field_signature, "C") == 0) {
>>>>> 644 int value = atoi(string_value);
>>>>> 645 java_mirror->char_field_put(fd.offset(), value);
>>>>> 646 } else if (strcmp(field_signature, "S") == 0) {
>>>>> 647 int value = atoi(string_value);
>>>>> 648 java_mirror->short_field_put(fd.offset(), value);
>>>>> 649 } else if (strcmp(field_signature, "Z") == 0) {
>>>>> 650 int value = atol(string_value);
>>>>> 651 java_mirror->bool_field_put(fd.offset(), value);
>>>>> 652 } else if (strcmp(field_signature, "J") == 0) {
>>>>> 653 jlong value;
>>>>> 654 if (sscanf(string_value, INT64_FORMAT,&value) != 1) {
>>>>> 655 fprintf(stderr, "Error parsing long: %s\n", string_value);
>>>>> 656 return;
>>>>> 657 }
>>>>> 658 java_mirror->long_field_put(fd.offset(), value);
>>>>> 659 } else if (strcmp(field_signature, "F") == 0) {
>>>>> 660 float value = atof(string_value);
>>>>> 661 java_mirror->float_field_put(fd.offset(), value);
>>>>> 662 } else if (strcmp(field_signature, "D") == 0) {
>>>>> 663 double value = atof(string_value);
>>>>> 664 java_mirror->double_field_put(fd.offset(), value);
>>>>> 665 } else if (strcmp(field_signature, "Ljava/lang/String;") == 0) {
>>>>> 666 Handle value = java_lang_String::create_from_str(string_value, CHECK);
>>>>> 667 java_mirror->obj_field_put(fd.offset(), value());
>>>>> 668 } else if (field_signature[0] == 'L') {
>>>>>
>>>>> Same here (not the put though)?
>>>>>
>>>>> Otherwise it looks good.
>>>>>
>>>>> -- Chris
>>>>>
>>>>>> Thanks
>>>>>> Yumin
>>>
>>
More information about the hotspot-compiler-dev
mailing list