On Wed, 18 Sep 2024 13:57:29 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> src/hotspot/share/memory/classLoaderMetaspace.cpp line 87:
>>
>>> 85: klass_alignment_words,
>>> 86: "class arena");
>>> 87: }
>>
>> As per my comment in the header file, change the code to this:
>>
>> ```c++
>> if (class_context != nullptr) {
>> // ... Same as in PR
>> } else {
>> _class_space_arena = _non_class_space_arena;
>> }
>
> Rather not, see reasoning under
> https://github.com/openjdk/jdk/pull/20677/files#r1754330432
Yes, I'd rather _class_space_arena be nullptr if not used.
>> src/hotspot/share/memory/classLoaderMetaspace.cpp line 115:
>>
>>> 113: if (wastage.is_nonempty()) {
>>> 114: non_class_space_arena()->deallocate(wastage);
>>> 115: }
>>
>> This code reads a bit strangely. I understand *what* it tries to do. It
>> tries to give back any wasted memory from either the class space arena *or*
>> the non class space arena to the non class space arena's freelist. I assume
>> that we do this since any wastage is presumably too small to be used by our
>> new 22-bit class pointers. However, this context will be lost on future
>> readers. It should have at least a comment in the `if
>> (wastage.is_nonempty())` clause explaining what we expect should happen and
>> why. For example:
>>
>> ```c++
>> // Any wasted memory is presumably too small for any class.
>> // Therefore, give it back to the non-class space arena's free list.
>
> Yes. Some background:
>
> - wastage can only occur for larger Klass* alignments (aka class space arena
> alignment property), so only for +COH (note to self, maybe assert)
> - wastage is, by definition, not aligned to the required Klass* alignment, so
> it cannot be reused. Yes, its probably also too small
>
> Yes, I will write a better comment.
Yes, this definitely needs a comment why since this is how we allocate small
chunks of wasted because of hyper-aligning Klasses in class space. Line 111
is somewhat surprising though. I didn't expect there to be wastage from
allocating to non-class-metaspace.
The unnerving bit of this is that CompressedKlassPointers::is_encodable() is
true for memory allocated here.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1768897591
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1768966812
