[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8176520: Improve the accuracy of the instance size in hprof heap dumps [v3]
From:       Serguei Spitsyn <sspitsyn () openjdk ! org>
Date:       2024-02-24 4:41:00
Message-ID: HkLQlQiwN_XaIX5qjTK4XHUZryA7GMpPhEjEWO6uY_o=.46fdeeaf-4fd4-4cf0-9c51-cf39318d1338 () github ! com
[Download RAW message or body]

On Sat, 17 Feb 2024 02:41:20 GMT, Alex Menkov <amenkov@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


[prev in list] [next in list] [prev in thread] [next in thread]