On Tue, 17 Sep 2024 09:59:49 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> src/hotspot/share/memory/classLoaderMetaspace.hpp line 81:
>>
>>> 79: metaspace::MetaspaceArena* class_space_arena() const { return
>>> _class_space_arena; }
>>> 80:
>>> 81: bool have_class_space_arena() const { return _class_space_arena !=
>>> nullptr; }
>>
>> This is unnecessary. Instead of having this and having to remember to check
>> for nullness each time, just change the `_class_space_arena` to point to the
>> same arena as the `_non_class_space_arena` does when we run with
>> `-XX:-UseCompressedClassPointers`
>
> I'd prefer not to.
>
> This logic (when -UCCP class space arena is NULL, with the implicit
> assumption that both are different entities) has been in there forever, and
> changing that is out of scope for and unrelated to this PR. I am not sure
> what will break if I change this but don't want to risk test errors at this
> point (one example, reporting would have to be adapted to recognize that both
> arenas are the same, and there are plenty of tests that would also need to be
> fixd).
>
> This can be done in a follow-up RFE if necessary.
OK, that's fine.
>> src/hotspot/share/memory/metaspace.cpp line 656:
>>
>>> 654: // Adjust size of the compressed class space.
>>> 655:
>>> 656: const size_t res_align = reserve_alignment();
>>
>> Can you change the name to `root_chunk_size`?
>
> It feels wrong, since this is a deeply hidden implementation detail.\
>
> I will remove this temporary variable, which will also make the diff smaller.
Sounds OK, I wanted the name change to indicate that "hey, deep impl detail
where we use this to mean something else".
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1762993568
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1762994772
