RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal

[Roman Kennke]

On Thu, 4 Apr 2024 09:45:58 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> We have a few places that uses the terms `KlassObj` and `klassOop` when referring to Klasses. This is old code from before the PermGen removal, when Klasses also were Java objects.
> 
> These names tripped me up when I was reading the heap heapInspection.cpp and first though we were mixing the klass *mirror* objects and klass pointers in the hash code calculation:
> 
>    // An aligned reference address (typically the least
>    // address in the perm gen) used for hashing klass
>    // objects.
>    HeapWord* _ref;
> ...
> _ref = (HeapWord*) Universe::boolArrayKlassObj();
> ...
> uint KlassInfoTable::hash(const Klass* p) {
>   return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
> }
> 
> 
> I propose that we rename these functions (and stop casting the Klass* to a (HeapWord*)).
> 
> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this through our lower tiers.

This is a useful change, it has tripped me up a couple of times, too. Change mostly looks good, just a few minor suggestions.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 1202:

> 1200:   ldrw(scan_temp, Address(recv_klass, Klass::vtable_length_offset()));
> 1201: 
> 1202:   // %%% Could store the aligned, prescaled offset in the klassoop.

Unrelated, but what's the point of the %%% in all those comments? Might want to remove that, while you're there.

src/hotspot/share/memory/heapInspection.cpp line 173:

> 171: KlassInfoTable::KlassInfoTable(bool add_all_classes) {
> 172:   _size_of_instances_in_words = 0;
> 173:   _ref = (uintptr_t) Universe::boolArrayKlass();

It seems weird (non-obvious) to cast to uintptr_t here. I see it is only used in KlassInfoTable::hash(), which is weird, too. I am not sure that this even does a useful hashing. Might be worth to get rid of the whole thing and use the [fastHash](https://github.com/rkennke/jdk/blob/JDK-8305896/src/hotspot/share/utilities/fastHash.hpp) stuff that @rose00 proposed for Lilliput. Perhaps in a follow-up. I'd probably either cast to void* or Klass*, or cast to uintptr_t as you did and remove the unnecessary cast in ::hash().

-------------

Changes requested by rkennke (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18618#pullrequestreview-1979371014
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551366745
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551384583