8320215: HeapDumper can use DumpWriter buffer during merge

[alexmenkov]

The fix updates HeapMerger to use writer buffer (no need to copy memory, also writer buffer is 1MB instead of 4KB).
Additionally fixed small issue in FileWriter (looks like ssize_t instead of size_t is a typo, the argument should be unsigned)

Testing: all HeapDump-related tests on Oracle supported platforms

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8320215: HeapDumper can use DumpWriter buffer during merge (Enhancement - P4)

Reviewers

• Serguei Spitsyn (@sspitsyn - Reviewer)
• Yi Yang (@y1yang0 - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18850/head:pull/18850
$ git checkout pull/18850

Update a local copy of the PR:
$ git checkout pull/18850
$ git pull https://git.openjdk.org/jdk.git pull/18850/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18850

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18850.diff

Webrev

Link to Webrev Comment

[alexmenkov]

/label add serviceability

[bridgekeeper]

👋 Welcome back amenkov! 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.

[openjdk]

@alexmenkov 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:

8320215: HeapDumper can use DumpWriter buffer during merge

Reviewed-by: sspitsyn, yyang

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 556 new commits pushed to the master branch:

• 789ac8b: 8333189: Make sure clang on linux uses lld as linker
• c8eea59: 8332919: SA PointerLocation needs to print a newline after dumping java thread info for JNI Local Ref
• bc7d9e3: 8333013: Update vmTestbase/nsk/share/LocalProcess.java to don't use finalization
• 03b7a85: 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state
• 43a2f17: 8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage
• fed2b56: 8320999: RISC-V: C2 RotateLeftV
• 6cda4c5: 8321543: Update NSS to version 3.96
• c003c12: 8331865: Consolidate size and alignment checks in LayoutPath
• 6d718ae: 8324341: Remove redundant preprocessor #if's checks
• 9b64ece: 8332904: ubsan ppc64le: c1_LIRGenerator_ppc.cpp:581:21: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long int'
• ... and 546 more: https://git.openjdk.org/jdk/compare/e57a322d7076474806458cc4b796bdb874e8e92e...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.

[openjdk]

@alexmenkov
The serviceability label was successfully added.

[mlbridge]

Webrevs

• 00: Full (dae43205)

[sspitsyn]

Looks good.

[alexmenkov]

Ping. Can I get second review please.

[plummercj]

Why flush on each iteration instead of just once after you are done with the loop?

[alexmenkov]

Standard way to use AbstractDumpWriter is to use AbstractDumpWriter::write_XXX methods (they all call write_raw(const void* s, size_t len) which copies data from the provided memory to _writer->buffer(), and flushes when the buffer is full)
This code uses writer internals - on each iteration we read up to 1MB from segment_fs directly to _writer->buffer(), so we need to flush it before performing next read, otherwise data in the buffer will be overridden.

[y1yang0]

I remember experimenting with different buffer sizes and figuring out that 4KB was the sweet spot. We could potentially switch to 1MB, but it would be better if we had some benchmark numbers to back that up.

[alexmenkov]

I remember experimenting with different buffer sizes and figuring out that 4KB was the sweet spot. We could potentially switch to 1MB, but it would be better if we had some benchmark numbers to back that up.

My experiments shown that with the fix time of the merge decreased ~5%.
But this is not just change of the buffer size:
Before the fix merger (1) read segment_fs in 4K chunks, (2) wrote them (copying memory) to _writer->_buffer until the buffer is full (i.e. 256 read iterations) and then (3) flush _writer->_buffer;
With the fix (1) and (2) are replaced with single reading of 1MB directly to _writer->_buffer (no memory copy).

[y1yang0]

Thanks for the explanation, looks good then~

[plummercj]

Looks good.

[alexmenkov]

/integrate

[openjdk]

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

• 681137c: 8333131: Source launcher should work with service loader SPI
• 914423e: 8332899: RISC-V: add comment and make the code more readable (if possible) in MacroAssembler::movptr
• 5abc029: 8331877: JFR: Remove JIInliner framework
• d9e7b7e: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater
• 1e04ee6: 8331579: Reference to primitive type fails without error or warning
• 32ee252: 8333169: javac NullPointerException record.type
• e930bc1: 8329537: Nested and enclosing classes should be linked separately in breadcrumb navigation
• 79a78f0: 8333129: Move ShrinkHeapInSteps flag to Serial GC
• 2f2dc22: 8330981: ZGC: Should not dedup strings in the finalizer graph
• d481215: 8333005: Deadlock when setting or updating the inline cache
• ... and 590 more: https://git.openjdk.org/jdk/compare/e57a322d7076474806458cc4b796bdb874e8e92e...master

Your commit was automatically rebased without conflicts.

[openjdk]

@alexmenkov Pushed as commit e4fbb15.

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