Re: RFR: 8269685: Optimize HeapHprofBinWriter implementation [v9]

[Alex Menkov]

On Fri, 17 Sep 2021 01:51:26 GMT, Lin Zang <lz...@openjdk.org> wrote:

>> This PR rewrite the implementation of the HeapHprofBinWriter, which could 
>> simplify the logic of current implementation.
>> please see detail description at 
>> https://bugs.openjdk.java.net/browse/JDK-8269685.
>
> Lin Zang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   code refine

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
 line 487:

> 485:     }
> 486: 
> 487:     protected int calculateOopDumpRecordSize(Oop oop) throws IOException 
> {

I think the method cannot throw IOException (as well as other calculate.. 
methods)

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
 line 489:

> 487:     protected int calculateOopDumpRecordSize(Oop oop) throws IOException 
> {
> 488:         if (oop instanceof TypeArray) {
> 489:             return calculatePrimitiveArrayDumpRecordSize((TypeArray)oop);

Suggestion:

        if (oop instanceof TypeArray taOop) {
            return calculatePrimitiveArrayDumpRecordSize(taOop);

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
 line 496:

> 494:             if (bottomType instanceof InstanceKlass ||
> 495:                 bottomType instanceof TypeArrayKlass) {
> 496:                 return calculateObjectArrayDumpRecordSize((ObjArray)oop);

Suggestion:

        } else if (oop instanceof ObjArray oaOop) {
           ...
                return calculateObjectArrayDumpRecordSize(oaOop);

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
 line 502:

> 500:             }
> 501:         } else if (oop instanceof Instance) {
> 502:             Instance instance = (Instance)oop;

Suggestion:

        } else if (oop instanceof Instance instance) {

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
 line 535:

> 533:         int size = (int)BYTE_SIZE + (int)INT_SIZE + (int)OBJ_ID_SIZE * 2;
> 534:         if (k instanceof InstanceKlass) {
> 535:             InstanceKlass ik = (InstanceKlass) k;

Suggestion:

        if (k instanceof InstanceKlass ik) {

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
 line 566:

> 564:         size += SHORT_SIZE;
> 565:         for (Iterator<Field> itr = fields.iterator(); itr.hasNext();) {
> 566:             Field field = itr.next();

Suggestion:

        for (Field field: fields) {

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
 line 567:

> 565:         for (Iterator<Field> itr = fields.iterator(); itr.hasNext();) {
> 566:             Field field = itr.next();
> 567:             char typeCode = (char) field.getSignature().getByteAt(0);

unused variable

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
 line 578:

> 576:     }
> 577: 
> 578:     private int calculateFieldDumpRecordSize(Field field, Oop oop) 
> throws IOException {

This method is very similar to getSizeForFields
I think it makes sense to introduce method getSizeForField(Field) and use it 
here and in getSizeForFields
And I'd simplified the code to something like
Suggestion:

    private int getSizeForField(Field field)  {
        char typeCode = (char)field.getSignature().getByteAt(0);
        switch (typeCode) {
            case JVM_SIGNATURE_BOOLEAN:
            case JVM_SIGNATURE_BYTE:
                return 1;
            case JVM_SIGNATURE_CHAR:
            case JVM_SIGNATURE_SHORT:
                return 2;
            case JVM_SIGNATURE_INT:
            case JVM_SIGNATURE_FLOAT:
                return 4;
            case JVM_SIGNATURE_CLASS:
            case JVM_SIGNATURE_ARRAY:
                return OBJ_ID_SIZE;
            case JVM_SIGNATURE_LONG:
            case JVM_SIGNATURE_DOUBLE:
                return 8;
            default:
                throw new RuntimeException("should not reach here");
        }
    }

-------------

PR: https://git.openjdk.java.net/jdk/pull/4666