JDK-8276731: Metaspace chunks are uncommitted twice

[tstuefe]

Small patch for a minor, benign error, which just eats up a bit of unnecessary processing time.

Metaspace returns memory to the OS by uncommitting free chunks when we unload classes. Today, we uncommit free metaspace chunks twice, and the second uncommit has no effect. Therefore, one of the uncommits is unnecessary.

In detail, when class unloading happens, we enter CLDG::purge():

• CLDG::purge()

  • delete each CLD in the _unloading list
    • which deletes the associated metaspace arena(s)
      • which causes the arenas to return all of their now unused chunks to the chunk manager (ChunkManager::return_chunk())
        • where the buddy allocator combines them with their neighbors if they are free
          • (A) and the resulting larger chunk gets uncommitted if it is larger than a commit granule

• then we pop back to CLDG::purge() and call Metaspace::purge()

  • (B) Metaspace::purge() iterates through all free chunks in the chunk manager, now maximally folded, and uncommits those which are larger than a commit granule.

(A) and (B) are redundant. (B) uncommits memory which had already been uncommitted in step (A). Either A or B could be dropped.

I opt for keeping (B) and dropping (A) since:

• it is the expected behavior. We expect Metaspace::purge() to do some purging.
• (B) is less work than (A) since space is maximally defragmented at this point. All free chunks have been processed already and are therefore maximally folded by the buddy allocator.

---

Progress

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

Issue

• JDK-8276731: Metaspace chunks are uncommitted twice

Reviewers

• Aleksey Shipilev (@shipilev - Reviewer)
• Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6277

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

Using diff file

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

[bridgekeeper]

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

@tstuefe 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 (6ff3bf99)

[shipilev]

This makes sense to me. Uncommitting when returning the chunk to ChunkManager (which I think implies the potential for reuse) is somewhat unexpected. The uncommits should quite probably be separate, as your patch currently does.

[openjdk]

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

8276731: Metaspace chunks are uncommitted twice

Reviewed-by: shade, coleenp

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

• 5c7f77c: 8276850: Remove outdated comment in HeapRegionManager::par_iterate
• 945f408: 8276098: Do precise BOT updates in G1 evacuation phase
• 8747882: 8276790: Rename GenericCDSFileMapHeader::_base_archive_path_offset
• 38e6d5d: 8276677: Malformed Javadoc inline tags in JDK source in javax/net/ssl
• a7dedb5: 8276772: Refine javax.lang.model docs
• 14d66bd: 8276654: element-list order is non deterministic
• 905e3e8: 8231490: Ugly racy writes to ZipUtils.defaultBuf
• e383d26: 8275199: Bogus warning generated for serializable records
• 7e73bca: 8276408: Deprecate Runtime.exec methods with a single string command line argument
• 75adf54: 8276306: jdk/jshell/CustomInputToolBuilder.java fails intermittently on storage acquisition
• ... and 25 more: https://git.openjdk.java.net/jdk/compare/c393ee8f598850379266bdba1f55af94744dbad1...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.

[tstuefe]

This makes sense to me. Uncommitting when returning the chunk to ChunkManager (which I think implies the potential for reuse) is somewhat unexpected. The uncommits should quite probably be separate, as your patch currently does.

Exactly. Thanks for the review!

[coleenp]

I think this is fine. But now get_chunk won't uncommit memory, so could there be a scenario where we need to uncommit chunks outside of class unloading? I sort of doubt it but haven't thought about it for a long time. What do you think?

[tstuefe]

I think this is fine. But now get_chunk won't uncommit memory, so could there be a scenario where we need to uncommit chunks outside of class unloading? I sort of doubt it but haven't thought about it for a long time. What do you think?

Hi Coleen, thanks for thinking about this again :)

No, there is no reason to uncommit outside of class unloading. It's actually the other way around: As you noticed, the only other caller of ChunkManager::return_chunk() is ChunkManager::get_chunk() where get_chunk() takes a large chunk from the list, splits it, returns one sub chunk to the caller and the remainder splinters back into the list by calling return_chunk(). Today, the splinter chunks would be uncommitted again, which is pointless. They are uncommitted at that point.

[coleenp]

Great. I like this change then.

[tstuefe]

Great. I like this change then.

Thank you Coleen!

[tstuefe]

/integrate

[openjdk]

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

• 5c7f77c: 8276850: Remove outdated comment in HeapRegionManager::par_iterate
• 945f408: 8276098: Do precise BOT updates in G1 evacuation phase
• 8747882: 8276790: Rename GenericCDSFileMapHeader::_base_archive_path_offset
• 38e6d5d: 8276677: Malformed Javadoc inline tags in JDK source in javax/net/ssl
• a7dedb5: 8276772: Refine javax.lang.model docs
• 14d66bd: 8276654: element-list order is non deterministic
• 905e3e8: 8231490: Ugly racy writes to ZipUtils.defaultBuf
• e383d26: 8275199: Bogus warning generated for serializable records
• 7e73bca: 8276408: Deprecate Runtime.exec methods with a single string command line argument
• 75adf54: 8276306: jdk/jshell/CustomInputToolBuilder.java fails intermittently on storage acquisition
• ... and 25 more: https://git.openjdk.java.net/jdk/compare/c393ee8f598850379266bdba1f55af94744dbad1...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tstuefe Pushed as commit 4bd5bfd.

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