8338526: Don't store abstract and interface Klasses in class metaspace #19157
Conversation
|
👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into |
|
@coleenp 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: 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 155 new commits pushed to the
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 |
openjdk
bot
added
hotspot
|
@coleenp this pull request can not be integrated into git checkout anon
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
openjdk
bot
removed
the
merge-conflict
|
@coleenp This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
coleenp
changed the title
Webrevs
|
liach
left a comment
liach
left a comment
There was a problem hiding this comment.
java.lang.invoke changes look good.
src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
Show resolved
Hide resolved
|
If I understand correctly, we already have limits on how many classes we can represent as compressed class-pointers. While this is nice for Lilliput, this is equally useful for non-Lilliput CCP, because addressable class-space doesn't get polluted by classes that never need to be encoded as CCP, and thus effectively increases the number of classes that we can address without resorting to -UseCompressedClassPointers. |
|
I am surprised that this patch is so small. I would have assumed a lot of code exists that unconditionally assumes we always can encode decode Klass*<->narrowKlass. I looked through the typical cases (eg Klass validation) and all of them seem to be okay. I will keep looking. |
|
Thanks for looking at this @tstuefe. I was pleased that the change was small but once we identify the classes as AbstractClass instead of Class in metaspace, it just falls out. I thought CDS would have more changes, but CDS is all in one space and doesn't differentiate. It's good that the only time we compress and uncompress klass pointers is when we get them out of an object and this should keep it that way. |
tstuefe
left a comment
tstuefe
left a comment
There was a problem hiding this comment.
Hi Coleen,
IIUC, the new "is_in_klass_space" function that now is present in all Metadata children only exists because of the template function in MetadataFactory, right? Just for the purpose of deallocation?
If so, see this proposed addition to your patch: https://gist.github.com/tstuefe/5111c735b12f6d9c3c1d32699d0820f6
This would make Metaspace::deallocate smarter - Metaspace knows whether a given pointer is in class space or not, it can do automatically the right thing. There should be no need to tell it how to deallocate that storage. (If you are worried, in debug builds there are also sanity checks).
If you do this, I think you could remove all variants of "is_in_klass_space" apart from the one in Klass.
|
Yes, is_in_klass_space was just to direct where to deallocate the metaspace pointer. In your patch isn't the contains metaspace call still very slow? Or I suppose for class space, it's not because it's a fixed space. But it's not an inlined call at all because I had to search in cpp files for the range check.
I sort of think it might be better for the outside runtime code to control this and the metaspace call assert if its wrong. |
…s MetaspaceObj::Type.
coleenp
left a comment
coleenp
left a comment
There was a problem hiding this comment.
Thanks for reviewing this and your comments Thomas.
No, I think my way is better and it will be needed anyway for TinyCP/Lilliput. We only need to do two address comparisons, that should be simple and fast. I opened a PR to separate the change, and in that PR I also inline the check. I don't think the costs for two address comparisons matter, not with the comparatively few deallocations that happen (few hundreds or few thousand). If deallocate is hot, we are using metaspace wrong. |
How about "needs_class_space" then? |
MethodData does a lot of deallocations from metaspace because it's allocated racily. It might be using Metaspace wrong. |
I think that should be okay. This should still be an exception. I have never seen that many deallocations happening in customer cases. |
|
@tstuefe Do you have more comments on this PR? |
Sorry, I was swamped the past days. I'll take a look tomorrow. |
tstuefe
left a comment
tstuefe
left a comment
There was a problem hiding this comment.
I still worry about unforeseen implications of not every Klass having an nKlass. It will have some implications on some of my Lilliput work. I wish we could have done this earlier.
But in any case, this seems to be a reasonable way forward. One remark inline, otherwise this looks mostly good to me.
| static bool is_compressed_klass_ptr(const Klass* k) { | ||
| return using_class_space() && (is_in_class_space(k) || is_in_shared_metaspace(k)); | ||
| } | ||
|
|
There was a problem hiding this comment.
I propose to drop this, and instead add a utility function to CompressedKlassPointers like this:
// Returns true if p falls into the narrow Klass encoding range
inline bool CompressedKlassPointers::is_in_encoding_range(const void* p) {
return _base != nullptr && p >= _base && p < (_base + _range);
}
(Probably the _base != nullptr could even be left out, since _range==0 and _base==nullptr for -UseCompressedClassPointers)
And then use that function in jfrTraceIdKlass.cpp. That file needs to use CompressedKlassPointers anyway because it needs to encode the Klass*.
This avoids having to rely on the exact composition of the memory regions inside the encoding range. What counts is whether the Klass pointer points into the narrow Klass encoding range.
Essentially, with CDS, memory looks like this:
encoding encoding
base end
|-----------------------------------------------------------------------|
|----CDS---| |--------------------class space---------------------------|
There was a problem hiding this comment.
oh yes that's much better and more precise. _base != nullptr isn't needed because p == nullptr will still return false for p < _base + range (both zero).
Thanks for the ascii art!
…rs::is_in_encoding_range.
|
Thanks for reviewing Ioi and Thomas, and thank you Thomas for the suggested changes. |
|
Going to push as commit ad10493.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
On 9/10/24 12:42, Coleen Phillimore wrote:
Thanks for reviewing Ioi and Thomas, and thank you Thomas for the suggested changes.
I'm a bit concerned about this one.
I'm working on megamorphic dispatch, and a uniform representation of
compressed class pointers allows me to squeeze klass+offset into a
single 64-bit word. This in turn allows fast and simple method
lookups. I need, at least, to be able to use compressed interface
pointers. If interfaces are "somewhere else", and thus incompressible,
I can't do what I need to do. If, however, klass and non-klass
metaspaces are contiguous I guess it'd be OK, if not ideal. I'd much
rather use compressed klass pointers without having to decode them.
All I need is a way to represent interface pointers in a compact way
in lookup tables, and to be able to get from compressed class pointers
to the address. As long as interface pointers are in a 32-bit range
and there's a fast way to get from compressed class to address that's
OK.
…--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
|
|
On 8 Oct 2024, at 4:07, Andrew Haley wrote:
On 9/10/24 12:42, Coleen Phillimore wrote:
> Thanks for reviewing Ioi and Thomas, and thank you Thomas for the suggested changes.
I'm a bit concerned about this one.
I'm working on megamorphic dispatch, and a uniform representation of
compressed class pointers allows me to squeeze klass+offset into a
single 64-bit word. This in turn allows fast and simple method
lookups. I need, at least, to be able to use compressed interface
pointers. If interfaces are "somewhere else", and thus incompressible,
I can't do what I need to do.
This is an interesting use of compressed klass pointers.
It looks like the compression needs to be partial, down to about 32 bits.
A motivation of this work is to compress concrete klass indexes into LESS than 32 bits, to take up less room in object layout.
So it looks like (a) having ALL klass IDs fit in 32 bits, and (b) having all CONCRETE klass IDs fit into a smaller part of the same range, would meet all requirements. Doing it right might require TWO new companion types for Klass*, a compact concrete klass ID, and a 32-bit klass ID (supertype to compact concrete klass ID). I think the larger one could be called narrowKlass (current name), and maybe the narrower concrete ID could be called narrowConcreteKlass or narrowestKlass or something like that.
(Because of CDS, and maybe other factors, some compact concrete klass IDs will actually point to interfaces or abstract classes. So narrowConcreteKlass suggests a property of its referents that isn’t exactly true.)
… If, however, klass and non-klass
metaspaces are contiguous I guess it'd be OK, if not ideal. I'd much
rather use compressed klass pointers without having to decode them.
The way I read this is to think about putting, not klass and non-klass, but concrete-klass and non-concrete-klass IDs in the same range. (By ID I mean a compressed Klass pointer, or alternatively an index into some sort of special table that we haven’t needed to invent.)
… All I need is a way to represent interface pointers in a compact way
in lookup tables, and to be able to get from compressed class pointers
to the address. As long as interface pointers are in a 32-bit range
and there's a fast way to get from compressed class to address that's
OK.
|
|
@theRealAph I do not know your idea regarding this matter but does compressing interface pointers independently from concrete class pointers help? |
|
If the interfaces had a compact numbering, and there were a side table mapping the compact numbers to interface Klass* pointers, then I think Andrew's code would still work (with natural adjustments). But managing such a side table is at least as complicated as just doing the pointer compression stuff we do. (Hence my comment that we haven't had to invent side tables yet.) Because of CDS I don't think we can treat concretes and abstracts (or even just classes and interfaces) as disjoint metadata types with separate independent compression tactics or representations. I think we need a subtype/supertype relation between the "narrowest" and merely "narrower" klass IDs. |
|
AFAIK, @tstuefe (currently on vacation) has a working prototype of a Klass-lookup-table with good performance and reasonable ‘management cost’. This would make make many things much simpler and also help solve this problem because it makes irrelevant where a Klass lives. Roman |
|
On 10/9/24 06:53, John Rose wrote:
It looks like the compression needs to be partial, down to about 32 bits.
Yes, that's true. Slightly fewer bits would be nice, for tags.
So it looks like (a) having ALL klass IDs fit in 32 bits, and (b)
having all CONCRETE klass IDs fit into a smaller part of the same
range, would meet all requirements. Doing it right might require TWO
new companion types for Klass*, a compact concrete klass ID, and a
32-bit klass ID (supertype to compact concrete klass ID). I think
the larger one could be called narrowKlass (current name), and maybe
the narrower concrete ID could be called narrowConcreteKlass or
narrowestKlass or something like that.
I see, I think.
Simply keeping all of metadata in a single 32-bit range would be very
useful. We could have instantiable Klasses in one region, followed by
non-instantiable Klasses.
(Because of CDS, and maybe other factors, some compact concrete
klass IDs will actually point to interfaces or abstract classes. So
narrowConcreteKlass suggests a property of its referents that isn’t
exactly true.)
The way I read this is to think about putting, not klass and
non-klass, but concrete-klass and non-concrete-klass IDs in the same
range. (By ID I mean a compressed Klass pointer, or alternatively an
index into some sort of special table that we haven’t needed to
invent.)
While I've now thought of a way to encode Klass pointers that doesn't
need them all to be in the same 32-bit range, it may well be very
useful in other contexts to ensure that they will be.
Maybe we could reduce the footprint of Klass instances. Right now,
though, my invokeinterface prototypes *increase* the size of Klass
instances, so I really want to have the possibility of using
compressed metadata pointers.
For what it's worth, I'm relieved that we're not yet making Klass
pointers a table index. Another chained load in the path of method
dispatch, at a time when I'm trying to get rid of chained loads, would
be a Bad Thing for me.
…--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
|
Of course that is possible, but it add complexity; and my goal is reducing complexity. |
Probably, yes. I can pack klass ID+ [iv]table offset into a word to identify a method, or use a compressed metadata pointer. But I have a data dependency on a table load to get from a compressed Klass ID of some sort to a
I think that's right. Notwithstanding any side tables etc., it would be nice to make sure that all metadata is reachable with a 4Gbyte offset from some base. It's not so hard to make sure that concrete and abstract |
Suddenly I seem to hear Boromir and Yoda, in unison, saying, "One does not simply." |
Yeah, I get that, and I'm sure @tstuefe has done a great job. But it goes to the fundamental theorem of software engineering (FTSE), Wheeler's statement that "We can solve any problem by introducing an extra level of indirection," often followed by "…except for the problem of too many levels of indirection." |
|
Sometimes, if you are very clever and determined, you can do the very non-simple thing of putting some items at the least level of indirection, and other items at further levels of indirection. You'd try to put stuff you need frequently closer to the root address. Currently everything is equally close to the root address, except that IIRC we try to put a few really performance sensitive things on the earliest cache lines. Splitting one Klass structure into a near and a far part is doable in principle, but the complexity adds up quickly, and there is also a problem deciding what to put in the near part. Specifically, you probably want some v-table entries, but not all v-table entries, in the near part. (Why not all? Because near space is relatively scarce. The near part might even be fixed in size, depending on design.) The bottom line for me: I like flatter data and shorter indirection chains. The main exception is that I ALSO like to separate hot from cold data, when the opportunity arises, and cold data is often at the end of an unused indirection. (I think there is relatively little cold data in metaspace, and it is probably already packed inside a pointer to an Array.) |
10 participants
This change stores InstanceKlass for interface and abstract classes in the non-class metaspace, since class metaspace will have limits on number of classes that can be represented when Lilliput changes go in. Classes that have no instances created for them don't require compressed class pointers. The generated LambdaForm classes are also AllStatic, and changing them to abstract moves them to non-class metaspace too. It's not technically great to make them abstract and not final but you can't have both. Java classfile access flags have no way of specifying something like AllStatic.
Tested with tier1-8.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19157/head:pull/19157$ git checkout pull/19157Update a local copy of the PR:
$ git checkout pull/19157$ git pull https://git.openjdk.org/jdk.git pull/19157/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19157View PR using the GUI difftool:
$ git pr show -t 19157Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19157.diff
Webrev
Link to Webrev Comment