8269685: Optimize HeapHprofBinWriter implementation #4666

linzang · 2021-07-02T12:05:35Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issues

JDK-8269685: Optimize HeapHprofBinWriter implementation
JDK-8262386: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java timed out ⚠️ Issue is not open.

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer) ⚠️ Review applies to de20431
Alex Menkov (@alexmenkov - Reviewer) ⚠️ Review applies to 1da3ab1

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4666/head:pull/4666
$ git checkout pull/4666

Update a local copy of the PR:
$ git checkout pull/4666
$ git pull https://git.openjdk.java.net/jdk pull/4666/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4666

View PR using the GUI difftool:
$ git pr show -t 4666

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4666.diff

bridgekeeper · 2021-07-02T12:06:26Z

👋 Welcome back lzang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

linzang · 2021-07-02T12:07:53Z

/issue 8262386

openjdk · 2021-07-02T12:08:05Z

@linzang The following label will be automatically applied to this pull request:

serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

openjdk · 2021-07-02T12:08:21Z

@linzang
Adding additional issue to issue list: 8262386: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java timed out.

mlbridge · 2021-07-02T12:12:15Z

Webrevs

linzang · 2021-08-02T14:32:28Z

Dear All,
May I ask your help to review this PR? it fix the issue JDK-8262386.
I believe this could be a replacement for #2803. I am planing to close it if this one looks good.

BRs,
Lin

linzang · 2021-08-23T07:27:11Z

Added the tests I have conducted:
Passed test tier1 (tier2 and tier3 not executed)
Tested to dump netbeans with heap size at ~500MB, ~700MB and 1GB, and tested w/o -gz option from 1-9. all passed.
A workload that generate different object to fill heap, I have tested with heap usage from 1GB to 4GB with different gz options. All passed.

sspitsyn · 2021-08-23T23:26:14Z

Hi Lin,

You did not answer my question about empty methods in the AbstractHeapGraphWriter.java.
It seems the following methods have to be abstract instead of being empty:

+    protected int calculateGlobalJNIHandlesDumpRecordSize() {
+        return 0;
+    }
+
+    protected int calculateJavaThreadsDumpRecordSize() {
+        return 0;
+    }
+
+    protected int calculateOopDumpRecordSize(Oop oop) throws IOException {
+        return 0;
+    }
. . . 
    // object field writers
protected void writeReferenceField(Oop oop, OopField field)
     throws IOException {
}
protected void writeByteField(Oop oop, ByteField field)
    throws IOException {
}
protected void writeCharField(Oop oop, CharField field)
    throws IOException {
}
protected void writeBooleanField(Oop oop, BooleanField field)
    throws IOException {
}
protected void writeShortField(Oop oop, ShortField field)
    throws IOException {
}
protected void writeIntField(Oop oop, IntField field)
    throws IOException {
}
protected void writeLongField(Oop oop, LongField field)
    throws IOException {
}
protected void writeFloatField(Oop oop, FloatField field)
    throws IOException {
}
protected void writeDoubleField(Oop oop, DoubleField field)
    throws IOException {
}
protected void writeObjectFooter(Oop oop) throws IOException {
}
protected void writeHeapFooter() throws IOException {
}
protected void writeHeapRecordPrologue() throws IOException {
} 
+    protected void writeHeapRecordPrologue(int size) throws IOException {
+    }
+
protected void writeHeapRecordEpilogue() throws IOException {
}

linzang · 2021-08-23T23:51:42Z

Hi Serguei (@sspitsyn),

So sorry that I missed your comments, it is strange that your review didn't show on github in this PR.
I make these methods empty in AbstractHeapGraphWriter rather than abstract so that there is no need to add these empty code in HeapGXLWriter, which is a subclass of AbstractHeapGraphWriter. I think this may help make the code change clean.
What do you think?

BRs,
Lin

sspitsyn · 2021-08-24T05:25:49Z

Lin, in order to see my inlined comment you need to look at "Files changed".
I still miss something. If these methods have to be empty then what is the purpose to have them?
Also, a couple of new empty methods are not used, so the question is why do you add them:
calculateGlobalJNIHandlesDumpRecordSize, calculateJavaThreadsDumpRecordSize

linzang · 2021-08-24T06:36:13Z

Dear Serguei,

Lin, in order to see my inlined comment you need to look at "Files changed".

I can not find your comments here either, would you like to help check whether you sumbited the inlined commit after review?

I still miss something. If these methods have to be empty then what is the purpose to have them?

The main reason is that both HeapHprofBinWriter and HeapGXLWriter are extended from AbstractHeapGraphWriter, and they both invoke method write() in AbstractHeapGraphWriter for heap iteration which invoke doObj(). And in method doObj(), it calls writeHeapRecordPrologue(calculateOopDumpRecordSize()) which should do nothing for HeapGXLWriter and do some work for HeapHprofBinWriter. So I made empty method writeHeapRecordPrologue(int) and calculateOopDumpRecordSize() in base class and have them implemented in HeapHprofBinWriter only.

Also, a couple of new empty methods are not used, so the question is why do you add them:
calculateGlobalJNIHandlesDumpRecordSize, calculateJavaThreadsDumpRecordSize

Thanks for point them out, I will remove these not used methods.

BRs,
Lin

sspitsyn · 2021-07-06T22:17:04Z

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/AbstractHeapGraphWriter.java

+    protected int calculateOopDumpRecordSize(Oop oop) throws IOException {
+        return 0;
+    }
+


Hi Lin,
I don't like these empty methods in this abstract class.
Should they be just abstract instead?
Thanks,
Serguei

sspitsyn · 2021-08-24T10:19:16Z

Lin, you are right. It occurred I did not submit my comment (while I was sure that I did it).

linzang · 2021-08-24T12:33:27Z

Hi Serguei,
I update the PR to make the calculateGlobalJNIHandlesDumpRecordSize() method abstract.
For other empty ones that already there in original implementation, I will leave them as they are. do you think it is OK?

BRs,
Lin

sspitsyn · 2021-09-13T23:48:57Z

Hi Serguei,
I update the PR to make the calculateGlobalJNIHandlesDumpRecordSize() method abstract.
For other empty ones that already there in original implementation, I will leave them as they are. do you think it is OK?

BRs,
Lin

Hi Lin,
Yes, I'm okay with that.
Thanks,
Serguei

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

linzang · 2021-09-14T14:23:41Z

Hi @sspitsyn,
Thanks for your review and comments, I will fix them asap.
BRs,
Lin

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

sspitsyn · 2021-09-16T16:11:44Z

Hi Lin,
Thank you for update!
I've added several comments with formatting nits.
Otherwise, it looks okay to me but I did not review everything deeply.
Thanks,
Serguei

linzang · 2021-09-17T01:47:30Z

Dear Serguei @sspitsyn ，
Very appericate for your review and comments, I just updated the PR based on your comments.
Thanks!
Lin

sspitsyn

Hi Lin,
Thank you for the update!
It looks good to me.
Thanks,
Serguei

openjdk · 2021-09-17T17:28:59Z

@linzang This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8269685: Optimize HeapHprofBinWriter implementation
8262386: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java timed out

Reviewed-by: sspitsyn, amenkov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 11 new commits pushed to the master branch:

5ec1cdc: 8274321: Standardize values of @SInCE tags in javax.lang.model
4838a2c: 8274143: Disable "invalid entry for security.provider.X" error message in log file when security.provider.X is empty
ab28db1: 8274312: ProblemList 2 serviceability/dcmd/gc tests with ZGC on macos-all
8c122af: 8274314: Typo in WatchService#poll(long timeout, TimeUnit unit) javadoc
9bc865d: 8273960: Redundant condition in Metadata.TypeComparator.compare
5756385: 8274273: Update testing docs for MacOS with Non-US locale
61ac53f: 8210927: JDB tests do not update source path after doing a redefine class
341de49: 8273492: Move evacuation failure handling into G1YoungCollector
13e9ea9: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125
753b256: 8274296: Update or Problem List tests which may fail with uiScale=2 on macOS
... and 1 more: https://git.openjdk.java.net/jdk/compare/0c050be64b7db297126c4dca6c7ebfc9f386b9db...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

linzang · 2021-09-23T07:07:56Z

Dear All,
May I ask your help to review? I believe this PR need one more approval. Thanks!

BRs,
Lin

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

sspitsyn · 2021-09-24T18:59:02Z

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

@@ -1346,38 +1311,37 @@ private long getAddressValue(Address addr) {
        return res;
    }

+    // get size in bytes (in stream) required for given field.
+    private int getSizeForField(Field field) {
+        char typeCode = (char) field.getSignature().getByteAt(0);


Space after cast (char) is not not needed.

sspitsyn · 2021-09-24T19:01:02Z

Lin, thank you for the update. I like suggestions/simplifications from Alex.

linzang · 2021-09-26T06:58:25Z

Thanks @sspitsyn and @alexmenkov for your review and approvel! I will push this PR after 1 day if there is no new comments.

BRs,
Lin

linzang · 2021-09-28T02:23:14Z

/integrate

openjdk · 2021-09-28T02:59:25Z

Going to push as commit 8876eae.
Since your change was applied there have been 26 commits pushed to the master branch:

c880b87: 8274367: Re-indent stack-trace examples for Throwable.printStackTrace
c4b52c7: 8271303: jcmd VM.cds {static, dynamic}_dump should print more info
5b660f3: 8274392: Suppress more warnings on non-serializable non-transient instance fields in java.sql.rowset
0865120: 8274345: make build-test-lib is broken
75404ea: 8267636: Bump minimum boot jdk to JDK 17
14100d5: 8274170: Add hooks for custom makefiles to augment jtreg test execution
daaa47e: 8274311: Make build.tools.jigsaw.GenGraphs more configurable
2cffe4c: 8274326: [macos] Ensure initialisation of sun/lwawt/macosx/CAccessibility in JavaComponentAccessibility.m
172900d: 8274255: Update javac messages to use "enum class" rather than "enum type"
b0983df: 8274074: SIGFPE with C2 compiled code with -XX:+StressGCM
... and 16 more: https://git.openjdk.java.net/jdk/compare/0c050be64b7db297126c4dca6c7ebfc9f386b9db...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-09-28T02:59:39Z

@linzang Pushed as commit 8876eae.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the rfr Pull request is ready for review label Jul 2, 2021

openjdk bot added the serviceability serviceability-dev@openjdk.org label Jul 2, 2021

linzang mentioned this pull request Jul 2, 2021

8262386: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java timed out #2803

Closed

3 tasks

linzang force-pushed the hprof branch from 5f439cc to e5ef8bd Compare July 5, 2021 09:30

linzang added 3 commits July 5, 2021 17:30

8269685: Optimize HeapHprofBinWriter implementation

a85d6b9

Merge branch 'master' into hprof

e5ef8bd

fix write size issue

59691b5

Merge branch 'master' into hprof

cb1cbeb

code clean up and remove useless methods

7b2a964

sspitsyn reviewed Aug 24, 2021

View reviewed changes

linzang added 2 commits August 24, 2021 20:34

make calculateGlobalJNIHandlesDumpRecordSize abstract

32e6b4c

Merge branch 'master' into hprof

e8b7858

sspitsyn reviewed Sep 14, 2021

View reviewed changes