Request for review (s) - 7198873

[Jon Masamitsu]

Mikael,

I like the idea of having some way to make CMS do a full
collection (or a CMS concurrent collection with class unloading)
for metadata but don't know how to do it.  This is a case where
the users has told CMS not to unload classes.  In the case where
that a safe thing to do, we don't want to hit the application with
class unloading periodically.  Doing that behind users' backs
drive them crazy.   Seems like we would want to do this for
the user who has said not to unload classes when the user
should really do class unloading.  How do we help the latter
user without punishing the former?

I think that a good case can be made for making CMS do
class unloading by default.   I think it was a bad decision to
make it off by default and this is a good time to fix it.
It may not even have a visible effect.  When we first
decided to make it off by default the remark phase was
serial and now it is parallel (by default).

Jon

On 09/27/12 00:27, Mikael Gerdin wrote:
> Jon,
>
> On 2012-09-26 18:44, Jon Masamitsu wrote:
>> Mikael,
>>
>> Do you want a default value (less than max uint) for MaxMetaspaceSize
>> before
>> this change goes back?
>
> While I see a value in setting some sort of "sensible" value for 
> MaxMetaspaceSize this is not really the exact issue I was worried about.
>
> Consider this scenario:
>
> * Person runs a Java app server/container with +UseConcMarkSweepGC 
> (but without +CMSClassUnloadingEnabled).
> * Some app is repeatedly deployed and un-deployed from the instance, 
> every time an app is un-deployed its class loader and classes are made 
> unreachable.
>
> In this case CMS would not opt to unload classes until 
> MaxMetaspaceSize is hit or the process/system runs out of memory.
>
> Maybe there could be some sort of threshold for when to trigger a full 
> collection (which includes class unloading) without that threshold 
> value being a limit on the amount of memory available for meta data?
>
> I know this is a complicated policy issue, and I agree that it would 
> be good if we could enable +CMSClassUnloadingEnabled and get rid of 
> the issue altogether. The question is whether or not performance 
> testing resources are available to evaluate that.
>
> /Mikael
>
>>
>> Jon
>>
>> On 9/25/2012 11:09 AM, Mikael Gerdin wrote:
>>> On 2012-09-25 18:23, Jon Masamitsu wrote:
>>>> Mikael,
>>>>
>>>> Thanks for the review.
>>>>
>>>> The expand_and_allocate() does not do a GC.  It expands
>>>> the Metaspace and does an allocation from the expanded
>>>> space.  Only if that fails does the CMS case fall through to
>>>> the full GC.
>>>
>>> Yes.
>>>
>>>>
>>>> The policy for CMS is
>>>>
>>>> 1) Hitting the HWM should start a concurrent collection if
>>>> CMS is doing class unloading.
>>>> 2) Always  expand the Metaspace and allocate from
>>>> the expanded space.
>>>> 3) If expanding the Metaspace does not provide any free
>>>> space, do a full GC to reclaim classloaders and class metadata
>>>> and then retry the allocation.
>>>
>>> But at what point does expanding the Metaspace not provide any free
>>> space? Is there some sort of back-off so that we don't just go ahead
>>> and allocate all available memory and then try to do a full gc when
>>> we've filled up the address space?
>>> I'm kind of concerned about the case with CMS without
>>> CMSClassUnloadingEnabled. What I'm mainly worried about is slow leaks
>>> and cases where an application loads a bunch of classes and then
>>> releases the java level references to them but does not unload them
>>> since we don't get to the expand_and_allocate returning null.
>>>
>>> /Mikael
>>>
>>>>
>>>> Jon
>>>>
>>>>
>>>> On 09/25/12 07:23, Mikael Gerdin wrote:
>>>>> Jon,
>>>>>
>>>>> On 2012-09-24 23:46, Jon Masamitsu wrote:
>>>>>> NPG: VM Does not unload classes with UseConcMarkSweepGC
>>>>>>
>>>>>> If CMS is not doing class unloading, don't start a concurrent
>>>>>> collection for classloader (and metadata) collection (since
>>>>>> it won't happen without class unloading).
>>>>>
>>>>> It looks like you still unconditionally call expand_and_allocate when
>>>>> running with CMS, no matter the value of CMSClassUnloadingEnabled.
>>>>> I think that the code:
>>>>>
>>>>>  213       // For CMS expand since the collection is going to be
>>>>> concurrent.
>>>>>  214       _result =
>>>>>  215 _loader_data->metaspace_non_null()->expand_and_allocate(_size,
>>>>> _mdtype);
>>>>>
>>>>> Should be inside the "if (CMSClassUnloadingEnabled)" and if running
>>>>> without it set then CMS users will have to take the hit of a stw full
>>>>> gc when running into the metadata threshold.
>>>>>
>>>>> /Mikael
>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jmasa/7198873/webrev.00/
>>>>>>
>>>>>> Also, refactored the code for readability and guarded extra
>>>>>> output with Verbose.
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>> Jon
>>>>>
>>>
>