8301564: Non-C-heap allocated ResourceHashtable keys and values must have trivial destructor

[iklam]

To ensure we don't have memory leaks or other more serious memory management bugs, I added static asserts to check that the K and V types for ResourceHashtableBase must have trivial destructors if the table is not C_HEAP allocated. Thanks to @JornVernee for the original assert code.

The asserts actually found a problem with ClassLoaderStatsClosure::StatsTable. The space used by the oop in the freed hashtable may be trashed when -XX:+CheckUnhandledOops is enabled, so live objects that reuse the same space may be corrupted. (I tried but couldn't get it to fail).

I also had to change CodeBuffer::SharedTrampolineRequests because it's V type is LinkListImpl<int>, which has a non-trivial destructor. (Note: in this case, the destructor doesn't do anything; we can probably make it go away with C++-20. See https://stackoverflow.com/questions/40094871/sfinae-away-a-destructor)

Testing with tier1~4 + builds-tier5

---

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-8301564: Non-C-heap allocated ResourceHashtable keys and values must have trivial destructor

Reviewers

• Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to e3ccac72
• Jorn Vernee (@JornVernee - Reviewer) ⚠️ Review applies to e3ccac72

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12355

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

Using diff file

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

[bridgekeeper]

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

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

• hotspot

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

• 02: Full - Incremental (2421a75e)
• 01: Full - Incremental (915f058b)
• 00: Full (e3ccac72)

[coleenp]

Looks good. Glad there weren't more hashtables breaking this assumption.

[coleenp]

There's an mtStatistics NMT category, can you use that for this? and the instance below?

[iklam]

Fixed.

[openjdk]

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

8301564: Non-C-heap allocated ResourceHashtable keys and values must have trivial destructor

Reviewed-by: coleenp, jvernee

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

[JornVernee]

This looks good, thanks.

I'll put this here for posterity: we discussed changing ResourceHashtable to always call the destructor of the key and value (sketch here). That would allow using K and V with non-trivial destructors as well. But, this discussion didn't reach a conclusion yet (in particular, there were some doubts expressed over whether a container should be calling destructors at all).

In the mean time, this patch improves upon the status quo.

[iklam]

Thanks @JornVernee and @coleenp for the review.
/integrate

[openjdk]

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

• b00b70c: 8286907: keytool should warn about weak PBE algorithms
• ee0f5b5: 8301392: Port fdlibm log1p to Java
• f696785: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter
• cf6b9eb: 8301637: ThreadLocalRandom.current().doubles().parallel() contention
• c647ae6: 8301149: Parallel: Refactor MutableNUMASpace::update_layout

Your commit was automatically rebased without conflicts.

[openjdk]

@iklam Pushed as commit 04278e6.

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