GC.heap_dump performance regression in Java 21

[Alex Menkov]

Hi Hannes,

The change looks very reasonable to me.
Field order is not important in the heap dump (the order just should be 
the same in class and instance subrecords).

And I think it would be better to fix original FieldStream (or introduce 
new HierarchicalFieldStream and use in for heap dumping first and then 
switch JVMTI to use it). This should improve performance of JVMTI 
functions as well.

AFAICS JVMTI uses FieldStream/FilteredFieldStream in 2 places: 
GetClassFields and heap walking functions.
GetClassFields needs fields in the order they occur in the class file 
and it has to reverse the order returned by FieldStream, so switch it to 
use forward field stream is straightforward.
For heap walking functions field index is calculated trickier (it 
includes count of fields in superclasses/interfaces).

We don't have good test coverage for heap dumping, there are some basic 
tests in test/hotspot/jtreg/serviceability.

regards,
Alex

On 02/10/23 11:49, Hannes Greule wrote:
> Hi,
> 
> recently, a performance regression of jcmd GC.heap_dump was brought to 
> my attention. I investigated the regression and tracked down 
> https://bugs.openjdk.org/browse/JDK-8292818 as the source of it.
> For reproduction, I used the code at [1] and ran it with `java -Xmx2G 
> CountPrimes`.
> In Java 17, jcmd CountPrimes GC.heap_dump -overwrite heap.hprof finishes 
> in 2-3 seconds. In Java 21, it almost takes 20 seconds instead.
> 
> Further analysis showed that the functions in InstanceKlass to get the 
> access flags of a field (identified by its index) now requires an 
> iteration of the fields. As FieldStream from reflectionUtils.hpp 
> accesses such data through the InstanceKlass with a given field index, 
> this results in quadratic complexity for each object that gets dumped.
> 
> I wrote a fix for this, with which it seems to finish even faster than 
> before the regression.
> 
> Before opening a Pull Request for it, however, I would like to know if 
> this change is even feasible.
> Based on the implementation in fieldStreams, I built a class 
> `HierarchicalFieldStream` to stream over fields of all InstanceKlasses 
> in a hiararchy, similar to how `FieldStream` in reflectionUtils is 
> implemented already.
> The most significant difference is that the `FieldStream` from 
> reflectionUtils iterates fields backwards, while the `JavaFieldStream` 
> from fieldStreams iterates forwards. That means using the 
> `JavaFieldStream` and my `HierarchicalFieldStream` directly results in 
> different heap dumps as the fields are dumped in their encounter order. 
>  From what I've found, this order isn't specified. The order in which 
> super types are visited remains the same.
> Is this an acceptable change?
> I decided against changing the implementation of `FieldStream` from 
> reflectionUtils as it is used in JVMTI code too.
> 
> You can find my suggested implementation at [2].
> 
> Please let me know what you think about it, and also let me know if 
> there are any relevant tests that I should run that don't run in GHA 
> already.
> 
> If you agree with my changes, I will open a bug report and create a PR.
> 
> Thanks,
> Hannes
> 
> [1] https://gist.github.com/SirYwell/73d8e3d679e5aa49a11ebefc868b4404
> [2] 
> https://github.com/SirYwell/jdk/commit/9814ca2aea8ebd7400e256b7430d3961a3692a83