On Wed, 11 Sep 2024 12:47:30 GMT, Johan Sjölen <[email protected]> wrote:
>> Roman Kennke has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix FullGCForwarding initialization
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1755111131
