On Mon, 6 Sep 2021 07:17:58 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 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
