RFR: 8317692: jcmd GC.heap_dump performance regression after JDK-8292818 [v2]

[Alex Menkov]

On Fri, 13 Oct 2023 19:08:19 GMT, Hannes Greule <hgreule at openjdk.org> wrote:

>> See the bug description for more information.
>> 
>> This implementation brings down the time to take a heap dump on the example application in the bug report to <2 seconds on my machine.
>
> Hannes Greule 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 five additional commits since the last revision:
> 
>  - whitespace
>  - add test to verify all instance fields are present in heap dump
>  - Merge remote-tracking branch 'upstream/master' into perf/fieldstream
>  - whitespaces
>  - Iterate fields forwards on thread dump

test/hotspot/jtreg/serviceability/HeapDump/FieldsInInstanceTest.java line 124:

> 122: 
> 123:             Iterable<JavaHeapObject> objects = snapshot.getThings()::asIterator;
> 124:             for (JavaHeapObject heapObj : objects) {

Instead if iteration over all dumped objects it would be faster to:
`
       (JavaObject)snapshot.findClass(className).getInstances(false).nextElement();
`

test/hotspot/jtreg/serviceability/HeapDump/FieldsInInstanceTest.java line 126:

> 124:             for (JavaHeapObject heapObj : objects) {
> 125:                 if (heapObj instanceof JavaObject javaObj) {
> 126:                     if (javaObj.getClazz().getName().endsWith("$B")) {

Suggestion:

                    if (javaObj.getClazz().getName().equals(FieldsInInstanceTarg.B.class.getName())) {

test/hotspot/jtreg/serviceability/HeapDump/FieldsInInstanceTest.java line 139:

> 137:                         Asserts.assertTrue(asString.contains("3"), "value for field A.a not found");
> 138:                         Asserts.assertTrue(asString.contains("Field"), "value for field A.s not found");
> 139:                         System.out.println(fields);

Suggestion:

                        log(fields);

And I think it makes sense to print field values before asserts (so they appear in the log if some assertion throw an exception)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16083#discussion_r1359022900
PR Review Comment: https://git.openjdk.org/jdk/pull/16083#discussion_r1359000757
PR Review Comment: https://git.openjdk.org/jdk/pull/16083#discussion_r1359005734