8338929: Make Metaspace::deallocate space-aware #20701

tstuefe · 2024-08-24T07:18:09Z

For ucoming Lilliput changes, as well as to simplify JDK-8338526, we should make Metaspace::deallocate space-aware (as in, instead of having to tell it where the pointed-to block resides, it should know it on its own).

As of now, callers need to tell the function of the deallocated block is in class space or in non-class metaspace. That is unnecessary and won't work for TinyCP. The deallocation function can do this better on itself by doing a range check on the class space range.

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-8338929: Make Metaspace::deallocate space-aware (Bug - P4)

Reviewers

Andrew Dinn (@adinn - Reviewer) Review applies to df8b43ce
Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20701

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-08-24T07:18:34Z

👋 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 · 2024-08-24T07:19:36Z

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

8338929: Make Metaspace::deallocate space-aware

Reviewed-by: coleenp, adinn

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

1ff5f8d: 8338440: Parallel: Improve fragmentation mitigation in Full GC
0f66710: 8338939: Simplify processing of hidden class names
b25095b: 8338728: Misc issues in memory layout javadoc
414d23c: 8338765: ScheduledThreadPoolExecutor struggles with extremely long delays
d5c6158: 8338389: [JFR] Long strings should be added to the string pool
2edf574: 8338814: [PPC64] Unify interface of cmpxchg for different types
aefdbdc: 8338727: RISC-V: Avoid synthetic data dependency in nmethod barrier on Ztso
b704bfa: 8298920: Improve microbenchmark build times
b8e8e96: 8338668: Test javax/swing/JFileChooser/8080628/bug8080628.java doesn't test for GTK L&F
cd9e241: 8336289: Obliterate most references to _snprintf in the Windows JDK
... and 8 more: https://git.openjdk.org/jdk/compare/20d8f58c92009a46dfb91b951e7d87b4cb8e8b41...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 · 2024-08-24T07:19:58Z

@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 · 2024-08-24T10:49:21Z

Webrevs

adinn

Looks good

tstuefe · 2024-08-26T09:03:55Z

Looks good

Thank you, Andrew!

coleenp

Thank you for doing this and I agree in principle but now I'm confused about these spaces.

coleenp · 2024-08-26T12:23:54Z

src/hotspot/share/memory/metaspace.hpp

+  static bool contains(const void* ptr) {
+    return is_in_shared_metaspace(ptr) || // in cds
+           is_in_class_space(ptr) ||      // in class space
+           is_in_nonclass_metaspace(ptr); // in one of the non-class regions?


So the shared_metaspace region, which is the memory for CDS is not in class_space? I suppose for deallocate this doesn't matter since this memory is never deallocated.

By terminology, it is not in class space.

Class space I (we? don't we all?) call the section that is governed by the metaspace allocator. The CDS region containing Klass is not in class space. It is, however, in the narrow Klass decoding range:

Klass decoding range: [CompressedKlassPointers::base() ... CompressedKlassPointers::base() + CompressedKlassPointers::range() )

that range contains the CDS klass range, followed (after a possible gap) by the traditional class space. If CDS is off, its just the class space.

There had been a nice ASCII art somewhere, but I cannot find it anymore; maybe someone removed it.

This comment should then be "or the CDS archive", since the function is contains() and includes the non-class metaspace. That is, is_in_shared_metaspace() includes all of the CDS archive. I don't think the CompressedKlassPointers:::range limit is in the middle of the CDS archive memory area, unless CDS was changed without me noticing to separate class and non-class metadata (I hope not).

coleenp · 2024-08-26T12:25:34Z

src/hotspot/share/memory/metaspace.hpp

+
+  // Returns true if pointer points into one of the metaspace regions, or
+  // into the class space.
+  static bool is_in_shared_metaspace(const void* ptr);


This comment doesn't make sense to me. Isn't this just pointing to the CDS region, which includes class and non-class data? But is this in the range of class space?

You are absolutely right, the comment is wrong. Sorry for the confusion. This is just the CDS portion.

coleenp · 2024-08-26T12:26:34Z

src/hotspot/share/memory/metaspace.hpp

+  static bool is_in_nonclass_metaspace(const void* ptr);
+
+  // Returns true if ptr points into class space, false if it doesn't or if
+  // there is no class space.


Add a comment that the class space is one contiguous allocated region, which is why this pointer compare is correct, unlike the non-class metaspace.

tstuefe · 2024-08-26T14:12:48Z

@coleenp I improved the comments. I think as a follow-up I will take a look at these functions and figure out which ones we really need, and maybe scrap the rest.

coleenp

Nit comment change and question for my own clarification, but this looks fine.

coleenp · 2024-08-26T14:46:31Z

src/hotspot/share/memory/metaspace.hpp

+  static bool contains(const void* ptr) {
+    return is_in_shared_metaspace(ptr) || // in cds
+           is_in_class_space(ptr) ||      // in class space
+           is_in_nonclass_metaspace(ptr); // in one of the non-class regions?


This comment should then be "or the CDS archive", since the function is contains() and includes the non-class metaspace. That is, is_in_shared_metaspace() includes all of the CDS archive. I don't think the CompressedKlassPointers:::range limit is in the middle of the CDS archive memory area, unless CDS was changed without me noticing to separate class and non-class metadata (I hope not).

coleenp · 2024-08-26T14:52:27Z

src/hotspot/share/memory/metaspace.hpp

+  // two address comparisons are enough.
+  static inline bool is_in_class_space(const void* ptr) {
+    return ptr < _class_space_end && ptr >= _class_space_start;
+  }


So this does not include the Klasses in the CDS archive, it really is only useful for deallocation. I don't need this to be a comment update, just making sure I follow.

coleenp · 2024-08-27T13:14:08Z

src/hotspot/share/memory/metaspace.hpp

-  static bool contains(const void* ptr);
-  static bool contains_non_shared(const void* ptr);
+  // Returns true if the pointer points into class space, non-class metaspace, or the
+  // Klass portion of the CDS archive.


This was the tiny comment nit. This doesn't return the "Klass portion of the" CDS archive so can you remove those words? Then it's good to go and I'll merge with it. Thanks!

Oh, I see. How about "metadata portion" then?

Oh, I see. How about "metadata portion" then?

Okay, will do.

Could you re-approve?

tstuefe · 2024-08-27T14:49:58Z

Hi @coleenp

This comment should then be "or the CDS archive", since the function is contains() and includes the non-class metaspace. That is, is_in_shared_metaspace() includes all of the CDS archive. I don't think the CompressedKlassPointers:::range limit is in the middle of the CDS archive memory area, unless CDS was changed without me noticing to separate class and non-class metadata (I hope not).

(GitHub, why can I not reply to comments inline? )

CDS also has heap parts, which are not in the Klass decoding range. That is why I named it the "Klass portion". I could also name it the "Metaspace" portion, but that is confusing, since its not in metaspace. "Metadata portion" maybe?

So this does not include the Klasses in the CDS archive, it really is only useful for deallocation. I don't need this to be a comment update, just making sure I follow.

Yes, that is correct.

coleenp

Thank you!

tstuefe · 2024-08-27T15:44:27Z

Thanks a lot @adinn and @coleenp

/integrate

openjdk · 2024-08-27T15:46:11Z

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

fa4ff78: 8338690: CompactNumberInstance.format incorrectly formats some numbers (few vs many)
1ff5f8d: 8338440: Parallel: Improve fragmentation mitigation in Full GC
0f66710: 8338939: Simplify processing of hidden class names
b25095b: 8338728: Misc issues in memory layout javadoc
414d23c: 8338765: ScheduledThreadPoolExecutor struggles with extremely long delays
d5c6158: 8338389: [JFR] Long strings should be added to the string pool
2edf574: 8338814: [PPC64] Unify interface of cmpxchg for different types
aefdbdc: 8338727: RISC-V: Avoid synthetic data dependency in nmethod barrier on Ztso
b704bfa: 8298920: Improve microbenchmark build times
b8e8e96: 8338668: Test javax/swing/JFileChooser/8080628/bug8080628.java doesn't test for GTK L&F
... and 9 more: https://git.openjdk.org/jdk/compare/20d8f58c92009a46dfb91b951e7d87b4cb8e8b41...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-27T15:46:17Z

@tstuefe Pushed as commit daf2617.

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

fast is-in-class-space check

5c74ccb

tstuefe marked this pull request as draft August 24, 2024 07:18

tstuefe changed the title ~~JDK-8338929> Make Metaspace::deallocate space-aware~~ JDK-8338929: Make Metaspace::deallocate space-aware Aug 24, 2024

openjdk bot changed the title ~~JDK-8338929: Make Metaspace::deallocate space-aware~~ 8338929: Make Metaspace::deallocate space-aware Aug 24, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Aug 24, 2024

copyrights

df8b43c

tstuefe marked this pull request as ready for review August 24, 2024 10:45

openjdk bot added the rfr Pull request is ready for review label Aug 24, 2024

tstuefe mentioned this pull request Aug 24, 2024

8338526: Don't store abstract and interface Klasses in class metaspace #19157

Closed

3 tasks

adinn approved these changes Aug 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 26, 2024

coleenp reviewed Aug 26, 2024

View reviewed changes

tstuefe added 2 commits August 26, 2024 16:06

Merge branch 'master' into make-deallocate-space-aware

f4077b0

improve comments

5a38893

openjdk bot removed the ready Pull request is ready to be integrated label Aug 26, 2024

coleenp approved these changes Aug 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 26, 2024

coleenp reviewed Aug 27, 2024

View reviewed changes

tiny comment change

0ed170f

openjdk bot removed the ready Pull request is ready to be integrated label Aug 27, 2024

coleenp approved these changes Aug 27, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 27, 2024

openjdk bot added the integrated Pull request has been integrated label Aug 27, 2024

openjdk bot closed this Aug 27, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 27, 2024

8338929: Make Metaspace::deallocate space-aware #20701

8338929: Make Metaspace::deallocate space-aware #20701

Uh oh!

Conversation

tstuefe commented Aug 24, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Aug 24, 2024

Uh oh!

openjdk bot commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 24, 2024

Uh oh!

mlbridge bot commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

adinn left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Aug 26, 2024

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Aug 26, 2024

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Aug 27, 2024

Uh oh!

openjdk bot commented Aug 27, 2024

Uh oh!

openjdk bot commented Aug 27, 2024

Uh oh!

Uh oh!

tstuefe commented Aug 24, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 24, 2024 •

edited

Loading

mlbridge bot commented Aug 24, 2024 •

edited

Loading

tstuefe commented Aug 27, 2024 •

edited

Loading