jdk 8335409: Can't allocate and retain memory from resource area in frame::oops_interpreted

The ResourceMark added in 8329665 to address the case of having to allocate extra memory for the _bit_mask, prevents code in the closure from allocating and retaining memory from the resource area across the closure, relying on some ResourceMark in scope further up the stack from frame::oops_interpreted_do(). There is in fact one case today in JFR code where this kind of allocation happens.

The amount of locals and expression stack entries a method can have before having to allocate extra memory for the _bit_mask is 4*64/2 = 128. This is already big enough that we almost never have to allocate. A test run through mach5 tiers1-6 shows only a handful of methods that fall into this case, and most are artificial ones created to trigger this condition. So moving the allocation to the C heap shouldn't have any performance penalty as the comment otherwise says. This comment dates back from 2002 where instead of 128 entries we could have only 32, considering 32 bits cpus as still in main use (see bug for more history details).

The current code in InterpreterOopMap::resource_copy() has a comment expecting the InterpreterOopMap object to be recently created and empty, but it also has an assert in the allocation case path where it considers the entry might be in use already. This assert actually looks wrong since a used InterpreterOopMap object will not necessarily contain a pointer to resource area memory in _bit_mask[0]. I added an example case in the bug details. In any case, since we don't have any such cases in the codebase I added an explicit assert to verify each InterpreterOopMap is only used one.

I tested the patch by running it through mach5 tiers 1-6.

Thanks, Patricio

Progress

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

Issue

JDK-8335409: Can't allocate and retain memory from resource area in frame::oops_interpreted_do oop closure after 8329665 (Bug - P2)

Reviewers

David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20012

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

Using diff file

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

Webrev

Link to Webrev Comment

Jul 03 '24 16:07 pchilano

:wave: Welcome back pchilanomate! 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.

Jul 03 '24 16:07 bridgekeeper[bot]

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

8335409: Can't allocate and retain memory from resource area in frame::oops_interpreted_do oop closure after 8329665

Reviewed-by: dholmes, stuefe, coleenp, shade

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

dcf4e0d51f392afe2711223484e932e3826e8864: 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt
1472124489c841642996ae984e21c533ffec8091: 8333364: Minor cleanup could be done in com.sun.crypto.provider
7e11fb702696df733ca89d325200f2e9414402d9: 8335688: Fix -Wzero-as-null-pointer-constant warnings from fflush calls in jvmti tests
531a6d85b00b88688668ab1ced0db6ce0214a5f1: 8335911: Document ccls indexer in doc/ide.md
0e0dfca21f64ecfcb3e5ed7cdc2a173834faa509: 8330806: test/hotspot/jtreg/compiler/c1/TestLargeMonitorOffset.java fails on ARM32
f3ff4f7427c3c3f5cb2a115a61462bb9d28de1cd: 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux
8f62f31dff564289a2422d58e8ecd5062d443b81: 8335906: [s390x] Test Failure: GTestWrapper.java
2a2964759c73b3b9ab6afaad109383c89952977b: 8334777: Test javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java failed with NullPointerException
564a72e1dba0f145600c8e7eff66992fbf294df0: 8335955: JDK-8335742 wrongly used a "JDK-" prefix in the problemlist bug number
9c7a6eabb93c570fdb74076edc931576ed6be3e0: 8312125: Refactor CDS enum class handling
... and 65 more: https://git.openjdk.org/jdk/compare/fac74b118f5fda4ec297e46238d34ce5b9be1e21...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.

Jul 03 '24 16:07 openjdk[bot]

@pchilano 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.

Jul 03 '24 16:07 openjdk[bot]

Webrevs

Jul 03 '24 16:07 mlbridge[bot]

Thanks for the reviews @dholmes-ora, @coleenp, @shipilev and @tstuefe!

Jul 10 '24 16:07 pchilano

/integrate

Jul 10 '24 16:07 pchilano

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

fb66716a1bc914db194c5b0b833cc2317704f166: 8331725: ubsan: pc may not always be the entry point for a VtableStub
fb9a227e02ebf826edb762283e15dd7e402f8433: 8313909: [JVMCI] assert(cp->tag_at(index).is_unresolved_klass()) in lookupKlassInPool
e6c5aa7a6cb54c647d261facdcffa6a410849627: 8336012: Fix usages of jtreg-reserved properties
e0fb949460d0c7e2ab1697a6466e7d4831a20a33: 8335779: JFR: Hide sleep events
537d20afbff255489a7b1bdb0410b9d1aba715b7: 8335766: Switch case with pattern matching and guard clause compiles inconsistently
a44b60c8c14ad998e51239f48e64779304aaac50: 8335778: runtime/ClassInitErrors/TestStackOverflowDuringInit.java fails on ppc64 platforms after JDK-8334545
b5909cabeef22954f4d9c642b1cbf288b3454562: 8323242: Remove vestigial DONT_USE_REGISTER_DEFINES
dcf4e0d51f392afe2711223484e932e3826e8864: 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt
1472124489c841642996ae984e21c533ffec8091: 8333364: Minor cleanup could be done in com.sun.crypto.provider
7e11fb702696df733ca89d325200f2e9414402d9: 8335688: Fix -Wzero-as-null-pointer-constant warnings from fflush calls in jvmti tests
... and 72 more: https://git.openjdk.org/jdk/compare/fac74b118f5fda4ec297e46238d34ce5b9be1e21...master

Your commit was automatically rebased without conflicts.

Jul 10 '24 16:07 openjdk[bot]

@pchilano Pushed as commit 7ab96c74e2c39f430a5c2f65a981da7314a2385b.

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

Jul 10 '24 16:07 openjdk[bot]

/backport :jdk23

Jul 15 '24 15:07 pchilano

@pchilano Could not automatically backport 7ab96c74 to openjdk/jdk due to conflicts in the following files:

src/hotspot/share/interpreter/oopMapCache.hpp

Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk. Note: these commands are just some suggestions and you can use other equivalent commands you know.

# Fetch the up-to-date version of the target branch
$ git fetch --no-tags https://git.openjdk.org/jdk.git jdk23:jdk23

# Check out the target branch and create your own branch to backport
$ git checkout jdk23
$ git checkout -b backport-pchilano-7ab96c74-jdk23

# Fetch the commit you want to backport
$ git fetch --no-tags https://git.openjdk.org/jdk.git 7ab96c74e2c39f430a5c2f65a981da7314a2385b

# Backport the commit
$ git cherry-pick --no-commit 7ab96c74e2c39f430a5c2f65a981da7314a2385b
# Resolve conflicts now

# Commit the files you have modified
$ git add files/with/resolved/conflicts
$ git commit -m 'Backport 7ab96c74e2c39f430a5c2f65a981da7314a2385b'

Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk with the title Backport 7ab96c74e2c39f430a5c2f65a981da7314a2385b.

Below you can find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 7ab96c74 from the openjdk/jdk repository.

The commit being backported was authored by Patricio Chilano Mateo on 10 Jul 2024 and was reviewed by David Holmes, Thomas Stuefe, Coleen Phillimore and Aleksey Shipilev.

Thanks!

Jul 15 '24 15:07 openjdk[bot]