On Thu, 4 Apr 2024 10:07:11 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> Stefan Karlsson has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review Roman
>
> 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().
I agree. I'll start by removing the redundant cast in `::hash()`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551554904
