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

[Jon Masamitsu]

On 6/13/13 1:07 AM, Erik Helin wrote:
> On 2013-06-12, Jon Masamitsu wrote:
>> 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"?
> I'm sorry, I should be more explicit when I write my emails :(
>
> Yes, we've decided to use 'C' instead of 'K' for all external names
> regarding compressed klass space (and compressed klass pointers). I'm
> going to send out a separate patch with these changes, but I should have
> mentioned that in this email to avoid any confusion.
>
> If you want, I can continue to keep 'K' for this change and then change
> all of the naming in a separate patch (that would be more consistent).
> Would you prefer to do it this way?

I don't have a preference since very quickly it will all
become consistent.

Jon

>
> Thanks,
> Erik
>
>>> - 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
>>>>>