8365165: Zap C-heap memory at delete/free

[shipilev]

We sometimes have a lifecycle problem with C-heap allocated objects. The most recent I dealt with is JDK-8364501. It would be convenient to have diagnostic code to zap the memory that is freed on C heap. We already do this for alloc/realloc. When NMT is enabled (which it is for debug builds), we can also do this for frees, as NMT tells us the size of the free-ed block.

This PR introduces a new diagnostic flag to match other Zap* flags we already have, puts the zapping on free path, and wraps alloc/realloc zapping with the flag as well. The last part is not really necessary, but it is nicer to wrap zapping code with a flag like this, so we can disable it for testing performance. JCStress routinely opts-out of most of the zapping to gain higher sampling throughput on fastdebug builds.

Additional testing:

• Linux AArch64 server fastdebug, selectively rolling back JDK-8364501 -- starts to immediately crash on reproducer
• Linux x86_64 server fastdebug, all (no new crashes, phew)
• Linux AArch64 server fastdebug, all (no new crashes, phew)

---

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-8365165: Zap C-heap memory at delete/free (Enhancement - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Kim Barrett (@kimbarrett - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26775

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back shade! 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]

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

8365165: Zap C-heap memory at delete/free

Reviewed-by: kvn, kbarrett

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

• 57210af: 8365555: Cleanup redundancies in jpackage implementation
• a70521c: 8364973: Add JVMTI stress testing mode
• b023fea: 8365558: Fix stub entry init and blob creation on Zero
• ... and 46 more: https://git.openjdk.org/jdk/compare/19a76a45e9c8616414830c865801660bc6761e92...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]

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

• hotspot-runtime

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.

[mlbridge]

Webrevs

• 00: Full (385542bf)

[kimbarrett]

I have this vague recollection that maybe we used to do something like this, and decided to stop
because it really badly hurt performance in some cases. I know debug builds aren't expected to
be performant, but there's slow and then there's really unpleasant to use. Maybe make this
default to false and require explicit opt-in?

[shipilev]

This is a legitimate concern. We have been optimizing/guarding zapping code over the years, because excessive zapping is sometimes not worth it. That said, the utility for diagnostic zapping lies in being enabled by default. If we had this zapping in place, JDK-8364501 would have been trivial to find. So we already know it is useful.

To estimate rough costs of doing this extra work, I ran Linux x86_64 server fastdebug tier1 with and without the patch, and here are the results:

# Before
62589.94s user 5358.93s system 4015% cpu 28:16.24 total
62453.49s user 5388.42s system 3993% cpu 28:18.60 total
62363.92s user 5347.49s system 3976% cpu 28:22.75 total

# After
62803.82s user 5350.01s system 3983% cpu 28:31.05 total
63868.84s user 5415.74s system 3997% cpu 28:33.04 total
63864.74s user 5521.71s system 4051% cpu 28:37.57 total

So there is an impact, but I will hard-pressed to call it really bad.

The upside for this PR is that we can now summarily turn off malloc/realloc/free zapping, if we want to.

[tstuefe]

+1. Ideally, os::malloc and friends should not be terribly hot. That's why we have custom allocators for heavy fine-grained use cases like C2.

ZapCHeap may be a bit misleading as a name, since all it does is zap on free.

Zap on malloc would also be useful. If we are worried about speed, zapping the 1-2 words would already give 95% of effect, since that is in high likelyhood the later location for some important struct members. And there is some probability that the libc touches memory in the vicinity of the block start during allocation, so it's probably already paged in.

[shipilev]

Zap on malloc would also be useful.

I don't understand. ZapCHeap, as implement in current PR, zaps on malloc as well. Well, actually, it just wraps the already existing zapping code with flag guards. I used to call the flag ZapFreeCHeap, but then realized we do malloc/realloc side already, so it just claimed those to be a part of the same zapping feature.

[tstuefe]

Okay, I see. So it's the other way around, we zap newly allocated memory, but not free memory. I thought you were doing that, according to your comment:

When NMT is enabled (which it is for debug builds), we can also do this for frees, as NMT tells us the size of the free-ed block.

[shipilev]

The PR does all three, take a look? Current code already zaps on malloc/realloc path, those paths are now under new ZapCHeap flag. New code zaps on free path, and it is also under new ZapCHeap flag. So in the end, ZapCHeap covers malloc/realloc/free, and thus it has a proper name.

[tstuefe]

Ah okay. I should not review on Friday evenings. All good, then.

[kimbarrett]

@shipilev Thanks for doing some performance testing. Yeah, that doesn't look too bad.

Change looks good.

[vnkozlov]

Looks good. I submitted testing.

[vnkozlov]

My testing passed. Good.

[kimbarrett]

@shipilev Thanks for doing some performance testing. Yeah, that doesn't look too bad.

Change looks good.

[shipilev]

Thank you all! I think we are in consensus this is a right thing to do. So I am integrating.

/integrate

[openjdk]

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

• 190e113: 8364263: HttpClient: Improve encapsulation of ProxyServer
• 166ea12: 8365543: UnixNativeDispatcher.init should lookup open64at and stat64at on AIX
• e7ca8c7: 8365436: ImageReaderTest fails when jmods directory not present
• ... and 51 more: https://git.openjdk.org/jdk/compare/19a76a45e9c8616414830c865801660bc6761e92...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit ca753eb.

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