Fwd: Re: RFR: 6830717: replay of compilations would help with debugging
yumin.qi at oracle.com
yumin.qi at oracle.com
Tue Nov 6 12:41:58 PST 2012
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
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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20121106/2a279054/attachment-0001.html
More information about the hotspot-compiler-dev
mailing list