RFR: 8013590: NPG: Add a memory pool MXBean for Metaspace

[Jon Masamitsu]

On 6/11/2013 8:27 AM, Erik Helin wrote:
> Hi Mikael and Jon,
>
> thanks for reviewing!
>
> I have updated the webrev. The changes from webrev.00 are:
> - Only exposing the compressed class space memory pool if compressed
>    classes are used.
> - Renamed the name of the pool to "Compressed Class Space" (was
>    "Compressed Klass Space", note the 'K').

There was a discussion about the way the flag UseCompressedKlassPointers 
was spelled.
I had advocated for keeping the "K" in Klass but after the other 
responses I thought
I had lost.  In particular I had thought the flag had existed before the 
NPG changes and
had been exposed to users and was wrong.

Why did you decide to keep the flag (and internal variable names) with 
the "K" but
change the name of the space to "Compressed Class Space"?


> - Renamed the variable _cks_pool to _compressed_class_pool.
> - Using max_uintx instead of _undefined_size.
> - Updated the test and the rest of the code to take these new change
>    into account.
>
> Please see new webrev at:
> http://cr.openjdk.java.net/~ehelin/8013590/webrev.01/

Rest looks fine.

Jon

>
> On 2013-06-04, Jon Masamitsu wrote:
>> http://cr.openjdk.java.net/~ehelin/8013590/webrev.00/test/gc/metaspace/TestMetaspaceMemoryPool.java.html
>>
>> The functions assertEquals(), assertTrue(), and assertDefined() seem
>> useful.  Is there
>> a more global place where they can be put so other can use them?
> I agree that assertTrue and assertEquals should be move to a separate
> file, but I think that assertDefined is mostly usable for tests dealing
> with MXMemoryBeans (since defined is not always the same as "differ from
> -1").
>
> I would like to do these changes as a separate change. Jon, what do you
> about that?
>
> Thanks,
> Erik
>
> On 2013-05-31, Mikael Gerdin wrote:
>> (merging the gc-dev and svc-dev threads)
>>
>> Hi Erik,
>>
>> On 2013-05-29 10:44, Erik Helin wrote:
>>> Hi all,
>>>
>>> this want sent to hotspot-gc-dev at openjdk.java.net, sending to
>>> serviceability-dev at openjdk.java.net as well since the change is about
>>> memory pools.
>>>
>>> This change adds two memory pools for metaspace, one for Metaspace and
>>> one for compressed klass space. The memory pool for compressed klass
>>> space will only have valus that differ from 0 or -1 (undefined) if
>>> compressed klass pointers are used.
>>>
>>> Question: Should I use an empty pool when compressed klass pointers are
>>> *not* used or should I just not expose the pool?
>>>
>>> This change also adds a manager for the pools: Metaspace Manager.
>>>
>>> I have also added a test that checks that the metaspace manager is
>>> present and that the two pools are present. The test also verifies that
>>> the compressed klass space pool act correct according to the
>>> UseCompressedKlass flag.
>>>
>>> The last time I added metaspace memory pools, it triggered some
>>> unforeseen bugs:
>>> - Two asserts in jmm_GetMemoryUsage that asserted that a memory pool was
>>>    either of heap type or had an undefined init/max size.
>>> - The jdk/test/java/lang/management/MemoryMXBean/MemoryTest.java failed
>>> - The service lock was taken out of order with the metaspace locks
>>>
>>> These bugs have all been fixed:
>>> - The asserts have been removed since they are no longer true
>>> - The test has been updated but is not part of this change since it is a
>>>    JDK change
>>> - This change does not make use of functions requiring the metaspace
>>>    lock. I had to remove some verification code in free_chunks_total to
>>>    ensure this.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~ehelin/8013590/webrev.00/
>> Overall I think your fix looks good but I disagree with your choice
>> of exposing an EmptyCompressedKlassSpacePool for the case of
>> -UseCompressedClassPointers.
>>
>> Given the dynamic nature of the MemoryManagerMXBeans and
>> MemoryPoolMXBeans it seems more true to the intentions of the API to
>> ony expose a MemoryPool if it contains interesting information.
>>
>> Generally I don't think users of the management APIs can (or should)
>> depend on the exact set of MemoryManagerMXBeans and
>> MemoryPoolMXBeans
>> available in a given VM instance.
>>
>> /Mikael
>>
>>> Testing:
>>> - One new jtreg test
>>> - JPRT
>>> - All the tests that failed in nighly testing last time now pass
>>>
>>> Thanks,
>>> Erik
>>>