On Fri, 13 Oct 2023 19:08:19 GMT, Hannes Greule <[email protected]> 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
