On Sat, 17 Feb 2024 02:41:20 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> The fix updates heap dumpers to report correct instance size value for
>> HPROF_GC_CLASS_DUMP records (currently it's reported as size of all instance
>> fields)
>>
>> Testing: tier1, tier2, tier5-svc
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> split test
The fix looks good in general.
I've posted a question about the instance size verification.
It applies to the SA test as well.
test/hotspot/jtreg/serviceability/HeapDump/HeapDumpInstanceSize.java line 39:
> 37: * @bug 8176520
> 38: * @summary Test that heap dumper reports correct instance size in
> HPROF_GC_CLASS_DUMP records
> 39: * @requires vm.jvmti
Q: Is the `vm.jvmti` really needed?
test/hotspot/jtreg/serviceability/HeapDump/HeapDumpInstanceSize.java line 88:
> 86: } else {
> 87: // non-array should have >0 instance size
> 88: if (instSize == 0) {
This check is not that strong. How were the real instance sizes verified?
Is it in the `HprofParser.parseAndVerify(fileDump)`?
test/hotspot/jtreg/serviceability/sa/HeapDumpInstanceSize.java line 41:
> 39: * @bug 8176520
> 40: * @summary Test that heap dumpers (VM and SA) report consistent instance
> size in HPROF_GC_CLASS_DUMP records
> 41: * @requires vm.jvmti
Q: Why is the `vm.jvmti` needed?
-------------
PR Review: https://git.openjdk.org/jdk/pull/17855#pullrequestreview-1899212106
PR Review Comment: https://git.openjdk.org/jdk/pull/17855#discussion_r1501340467
PR Review Comment: https://git.openjdk.org/jdk/pull/17855#discussion_r1501342401
PR Review Comment: https://git.openjdk.org/jdk/pull/17855#discussion_r1501342661
