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

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8287135: Calculation of jmm_GetMemoryUsage is wrong [v5]
From:       Thomas Stuefe <stuefe () openjdk ! org>
Date:       2022-06-29 6:40:47
Message-ID: _Tie8F_4wQKMyJesfie8UfyUlCBfOVmOhL9Lc4Q_nhE=.e8cdbe87-3b66-4528-b95b-e35b3d5e35c5 () github ! com
[Download RAW message or body]

On Mon, 27 Jun 2022 08:15:01 GMT, Yi Yang <yyang@openjdk.org> wrote:

> > It seems that calculation of \
> > MemoryMXBean.getNonHeapMemoryUsage(jmm_GetMemoryUsage) is wrong. 
> > Currently, `NonHeapUsage=CodeCache+Metaspace(ClassTypeSpace+NonClassTypeSpace)+CompressedClassSpace(ClassTypeSpace)`
> >  
> > ==> CodeHeap 'non-nmethods' 1532544 (Used)
> > ==> CodeHeap 'profiled nmethods' 0
> > ==> CodeHeap 'non-profiled nmethods' 13952
> > ==> Metaspace 506696
> > ==> Compressed Class Space 43312
> > init = 7667712(7488K) used = 2096504(2047K) committed = 8454144(8256K) max = \
> > -1(-1K) 
> > In this way, getNonHeapMemoryUsage is larger than it ought to be, it should be \
> > `NonHeapUsage = CodeCache + Metaspace`.
> 
> Yi Yang has updated the pull request incrementally with one additional commit since \
> the last revision: 
> address @tstuefe's comments; remove unnecessary declaration

I think @iklam meant something somewhat different. Since we remove the class space \
bean, we now need to rewrite those tests using that bean.

Lets say we create the following whitebox APIs

- long WB_getClassSpaceUsage
- long WB_getClassSpaceCommitted

Maybe also (to be symmetric, and to not have to mix bean code and this code in \
                test/hotspot/jtreg/vmTestbase/metaspace/gc/MemoryUsageTest.java:
- long WB_getMetaSpaceUsage
- long WB_getMetaSpaceCommitted

Maybe also:
- boolean WB_usesClassSpace()
- long WB_classSpaceMax()
though these can easily be implemented using existing whitebox method (there is one \
to get VM flags, so you could get   `UseCompressedClassPointers` and \
`CompressedClassSpaceSize` instead.

Now:

- In test/hotspot/jtreg/vmTestbase/metaspace/gc/MemoryUsageTest.java, you don't use \
beans anymore but those functions. You'd rewrite "getUsed()" and "getCommitted()" to \
use the new whitebox functions.

- In metaspce/ShrinkGrowTest, the bean is only used to confirm that class space is \
active. You can use WB_usesClassSpace for that, or the value of \
"UseCompressedClassPointers" flag.

- In test/hotspot/jtreg/vmTestbase/vm/mlvm/indy/stress/gc/lotsOfCallSites/Test.java, \
we use Compressed Class Bean to get Compressed Class usage and maximum size. YHere, \
you would have to exchange `classMetadataPoolMXB.getUsage();` with \
`WB_getClassSpaceUsage()` and `classMetadataPoolMXB.getUsage();` with \
`CompressedClassSpaceSize`

----

Note that since this RFR really grew in complexity compared to what you wanted to do \
originally, I'd be fine with dropping it altogether. If we continue, I think above \
proposal is the simplest way to go.

src/hotspot/share/services/memoryPool.cpp line 188:

> 186: 
> 187: // Note, any NonHeap pools would be counted into Non-Heap memory by \
>                 MemoryMXBean.getNonHeapMemoryUsage.
> 188: // Make sure that it behaves as you expected if you want to add a new type of \
> NonHeap pool

This comment is somewhat cryptic. Behave like what?

src/hotspot/share/services/memoryPool.cpp line 189:

> 187: // Note, any NonHeap pools would be counted into Non-Heap memory by \
>                 MemoryMXBean.getNonHeapMemoryUsage.
> 188: // Make sure that it behaves as you expected if you want to add a new type of \
>                 NonHeap pool
> 189: MetaspacePool::MetaspacePool() :

Can you please add a comment like
"Accounts for Metaspace (both class space and non-class space).
 Max usage is MaxMetaspaceSize. Current usage includes prematurely freed blocks.
 As far as this API is concerned, CompressedClassSpaceSize is ignored."

test/hotspot/jtreg/vmTestbase/nsk/share/gc/gp/GarbageUtils.java line 53:

> 51:             ANY (),
> 52:             HEAP("Java heap space"),
> 53:             METASPACE("Metaspace");

This chance is wrong, I think. We still have class space OOMEs.

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

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.org/jdk/pull/8831


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