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

[Coleen Phillimore]

On Thu, 9 Nov 2023 10:45:18 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> See the rationale and some profiles in the bug.
>> 
>> This PR implements simple caching for object instance dumping, which is overwhelmingly hottest path in heap dumping code. This allows to put the cache straight in `HeapObjectDumper`, which is effectively thread local. A simple hash table would then work to cache the class metadata.
>> 
>> Given a simple workload from the bug, these are the motivational improvements and historical perspective:
>> 
>> 
>> $ for I in `seq 1 3`; do java -XX:+UseParallelGC -XX:+HeapDumpAfterFullGC -Xms8g -Xmx8g HeapDump.java 2>&1 | grep created; rm *.hprof; done
>> 
>> # Baseline JDK 8u382
>> Heap dump file created [1440865228 bytes in 5.492 secs]
>> Heap dump file created [1440865228 bytes in 5.460 secs]
>> Heap dump file created [1440865228 bytes in 5.471 secs]
>> 
>> # Baseline JDK 11.0.21
>> Heap dump file created [1446043070 bytes in 5.088 secs]
>> Heap dump file created [1446042039 bytes in 5.096 secs]
>> Heap dump file created [1446042835 bytes in 5.081 secs]
>> 
>> # Baseline JDK 17.0.10
>> Heap dump file created [1446841531 bytes in 3.276 secs]
>> Heap dump file created [1446841673 bytes in 3.342 secs]
>> Heap dump file created [1446841590 bytes in 3.323 secs]
>> 
>> # Baseline JDK 21.0.2 EA
>> Heap dump file created [1447828697 bytes in 5.516 secs]
>> Heap dump file created [1447829756 bytes in 5.461 secs]
>> Heap dump file created [1447829539 bytes in 5.435 secs]
>> 
>> # Baseline JDK 22
>> Heap dump file created [1447285149 bytes in 5.232 secs]
>> Heap dump file created [1447285149 bytes in 5.211 secs]
>> Heap dump file created [1447284813 bytes in 5.222 secs]
>> 
>> # Patched JDK 22
>> Heap dump file created [1447285087 bytes in 1.071 secs]
>> Heap dump file created [1447285149 bytes in 1.073 secs]
>> Heap dump file created [1447285073 bytes in 1.058 secs]
>> 
>> 
>> This amounts to 5x improvement, 1.2 GB/sec dump rate (somewhat I/O limited), and the best heap dump performance across all JDK releases. I am planning to backport this patch where possible: at least to JDK 21u, but hopefully to JDK 17u as well.
>> 
>> There are other simple improvements, but this one is the top bottleneck.
>
> Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Limit the table size
>  - Merge branch 'master' into JDK-8319650-heapdump-cache-meta
>  - Fix

This looks good.  I have a couple of minor comments.

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.

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)

src/hotspot/share/services/heapDumper.cpp line 830:

> 828:   }
> 829: 
> 830:   typedef ResourceHashtable<void*, DumperClassCacheTableEntry*, TABLE_SIZE, AnyObj::C_HEAP, mtServiceability,

If the key is InstanceKlass, why not use InstanceKlass* as the key instead of void*.

src/hotspot/share/services/heapDumper.cpp line 1041:

> 1039: 
> 1040: // returns the size of the instance of the given class
> 1041: u4 DumperSupport::instance_size(Klass* k, DumperClassCacheTableEntry* class_cache_entry) {

Since you changed the parameters to instance_size(), can you make the argument InstanceKlass* instead of Klass* to remove the cast at 1045.  I believe all the callers already pass InstanceKlass*.

src/hotspot/share/services/heapDumper.cpp line 1126:

> 1124: // dump the raw values of the instance fields of the given object
> 1125: void DumperSupport::dump_instance_fields(AbstractDumpWriter* writer, oop o, DumperClassCacheTableEntry* class_cache_entry) {
> 1126:   assert(class_cache_entry, "Pre-condition: should be provided");

Needs explicit check against nullptr.

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16545#pullrequestreview-1722576207
PR Review Comment: https://git.openjdk.org/jdk/pull/16545#discussion_r1388081172
PR Review Comment: https://git.openjdk.org/jdk/pull/16545#discussion_r1388031769
PR Review Comment: https://git.openjdk.org/jdk/pull/16545#discussion_r1388027181
PR Review Comment: https://git.openjdk.org/jdk/pull/16545#discussion_r1388029365
PR Review Comment: https://git.openjdk.org/jdk/pull/16545#discussion_r1388068721