Request for review (s) - 7198873

[Mikael Gerdin]

Jon,

On 2012-09-27 17:24, Jon Masamitsu wrote:
> 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.

Isn't
"heap->collect_as_vm_thread(GCCause::_metadata_GC_threshold);"
exactly what causes CMS to do a STW full collection (which includes 
unloading classes)?

> 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?

But the net effect will be that before perm removal CMS did unload 
classes without +CMSClassUnloadingEnabled but did it as a STW full gc.
Now that won't happen until we've used up a lot of machine resources, 
which we then don't return because we don't hand back memory for unused 
chunks to the OS, right?
To tell the VM to not unload classses at all would be to use 
-ClassUnloading (or -Xnoclassgc).
Hitting the user with class unloading should only need to happen 
periodically if the user loads a lot of new classes after "warmup".
If the user then deems it unacceptable to get STW full gcs when 
loading/unloading a lot of classes he can enable +CMSClassUnloadingEnabled.


>
> 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).

Agreed, but it seems like a larger effort to measure the impact of that 
setting.

/Mikael

>
> 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
>>>>>>
>>>>
>>