RFR: 8319650: Improve heap dump performance with class metadata caching [v3]

[Coleen Phillimore]

On Thu, 9 Nov 2023 14:56:56 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> src/hotspot/share/services/heapDumper.cpp line 813:
>> 
>>> 811: };
>>> 812: 
>>> 813: class DumperClassCacheTable : public StackObj {
>> 
>> Technically it's a ValueObj inside a StackObj, so don't really need to have StackObj here. To me, that it's means that you're going to use it as a local variable in a method, when it's really embedded in StackObj.  One can argue all ways on this though, it's just my opinion.
>
> Yeah. What is the proper idiom for "I do not want this to be allocated (in C heap or anywhere else) without NMT knowing"? `StackObj` is the closest I can get nowadays, I think. `ValueObj` is gone since JDK 11: https://bugs.openjdk.org/browse/JDK-8199282

yes, everything without a base class is a ValueObj and there's a link-time error if you try to call 'new' on it. The ValueObj base class was problematic.  Now it's just omitted.

>> src/hotspot/share/services/heapDumper.cpp line 825:
>> 
>>> 823:   static constexpr int CACHE_TOP = 256;
>>> 824: 
>>> 825:   static unsigned int ptr_hash(void* const& s1) {
>> 
>> Should we change primitive_hash to this? (not this PR)
>
> Maybe? Need to brush up my knowledge on non-power-of-two hash tables. This thing just copies the hash we have in other places :) But since we are looking at it, I see no grand point in overriding the hash code here, and we can just use the default one. When/if default one improves, this one should improve as well.

The reason I asked is that we use this primitive_hash for a lot of hashtables of InstanceKlass and Symbol so if your experiments saw too many collisions, it would be good to change for everything.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16545#discussion_r1388159937
PR Review Comment: https://git.openjdk.org/jdk/pull/16545#discussion_r1388162639