RFR: 6830717: replay of compilations would help with debugging
yumin.qi at oracle.com
yumin.qi at oracle.com
Thu Nov 8 12:29:48 PST 2012
Thanks for the review.
Yumin
On 11/8/2012 11:31 AM, Christian Thalinger wrote:
> 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