RFR: 8269685: Optimize HeapHprofBinWriter implementation [v7]

[Serguei Spitsyn]

On Mon, 6 Sep 2021 07:17:58 GMT, Lin Zang <lzang at 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 with a new target base due to a merge or a rebase. The pull request now contains seven commits:
> 
>  - Merge branch 'master' into hprof
>  - make calculateGlobalJNIHandlesDumpRecordSize abstract
>  - code clean up and remove useless methods
>  - Merge branch 'master' into hprof
>  - fix write size issue
>  - Merge branch 'master' into hprof
>  - 8269685: Optimize HeapHprofBinWriter implementation

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

> 521:                 }
> 522:                 return calculateInstanceDumpRecordSize(instance);
> 523:             }

The code above is really confusing.
You have these methods defined (Q: BTW, why are these methods not defined close to each other?):

+    private int calculateStringDumpRecordSize(Instance instance) {
+        return calculateInstanceDumpRecordSize(instance);
+    }
. . .
+    private int calculateThreadDumpRecordSize(Instance instance) {
+        return calculateInstanceDumpRecordSize(instance);
+    }

So, the methods `calculateStringDumpRecordSize()` and `calculateThreadDumpRecordSize()` are the same as the `calculateInstanceDumpRecordSize()`.

Then you have the code:

+        } else if (oop instanceof Instance) {
+            Instance instance = (Instance) oop;
+            Klass klass = instance.getKlass();
+            Symbol name = klass.getName();
+            if (name.equals(javaLangString)) {
+                return calculateStringDumpRecordSize(instance);
+            } else if (name.equals(javaLangClass)) {
+                return calculateClassInstanceDumpRecordSize(instance);
+            } else if (name.equals(javaLangThread)) {
+                return calculateThreadDumpRecordSize(instance);
+            } else {
+                klass = klass.getSuper();
+                while (klass != null) {
+                    name = klass.getName();
+                    if (name.equals(javaLangThread)) {
+                        return calculateThreadDumpRecordSize(instance);
+                    }
+                    klass = klass.getSuper();
+                }
+                return calculateInstanceDumpRecordSize(instance);
+            }
+        } else {

You can simplify the above with something like this:

+        } else if (oop instanceof Instance) {
+            Instance instance = (Instance) oop;
+            Klass klass = instance.getKlass();
+            Symbol name = klass.getName();
+            if (name.equals(javaLangString) ||
+                name.equals(javaLangClass) ||
+                name.equals(javaLangThread)) {
+                return calculateClassInstanceDumpRecordSize(instance);
+            } else {
+                return calculateInstanceDumpRecordSize(instance);
+            }
+        } else {

And my guess is that this simplified code can be simplified even further (Q: is it right?): 

+        } else if (oop instanceof Instance) {
+            Instance instance = (Instance) oop;
+                return calculateInstanceDumpRecordSize(instance);
+            }
+        } else {

And the methods `calculateStringDumpRecordSize()` and `calculateThreadDumpRecordSize()` can be removed as unneeded.

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

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