Request for review: JDK-8005602 NPG: classunloading does not happen while CMS GC with -XX:+CMSClassUnloadingEnabled is used

[Stefan Karlsson]

On 03/12/2013 10:44 AM, Mikael Gerdin wrote:
>
>
> On 2013-03-11 17:32, Jon Masamitsu wrote:
>>
>>
>> On 03/11/13 09:16, Mikael Gerdin wrote:
>>> Jon,
>>>
>>> On 2013-03-11 16:24, Jon Masamitsu wrote:
>>>> Mikael,
>>>>
>>>> Changes look good.
>>> Thanks
>>>
>>>>
>>>> Your change of the assertion at the end of
>>>> Metaspace compute_new_size brought
>>>> something to mind.  This is separate from
>>>> your changes, but do you think the
>>>> Metaspace compute_new_size() should take
>>>> the expand_lock()?  As you note classes are
>>>> being loaded and metadata is being allocated.
>>>> The high-water-mark (capacity_until_GC) is
>>>> being changed in compute_new_size() and
>>>> being read in the should_expand() method.
>>>> I don't think it will make a difference.
>>>
>>> It's interesting that you suggest this.
>>> I had a MutexLocker on the expand_lock() in my earlier versions but I
>>> had completely forgotten why I put it there.
>>> In the end I didn't move any of the calls to compute_new_size so I
>>> thought I'd skip the locking since I couldn't see any new obvious
>>> problems with it removed.
>>
>> That's fine with me.
>>
>>>
>>> But in principle I agree that we should probably hold expand_lock() in
>>> compute_new_size, but only as long as we only look at the metaspace on
>>> the level which is protected by that lock.
>>> If we start looking at individual chunks and their usage we'll need to
>>> take the appropriate metaspace_lock and not only the expand_lock.
>>
>> Right.  compute_new_size() is more globals than any particular
>> Metaspace so only the expand_lock would be appropriate at
>> this point.
>>
>> Again, looks good.
>
> Thanks Jon.
>
> I need one more review for this change. Any takers?

Mikael guided me through these changes, and I think this looks good.

Though, I would prefer if the print changes were pushed in a separate 
changeset.

StefanK

>
> /Mikael
>
>>
>> Jon
>>
>>>
>>> /Mikael
>>>
>>>>
>>>> Jon
>>>>
>>>> On 03/11/13 06:08, Mikael Gerdin wrote:
>>>>> Anyone?
>>>>>
>>>>> On 2013-03-04 15:44, Mikael Gerdin wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this change to enable CMS to hand back memory for
>>>>>> unloaded
>>>>>> classes when running in concurrent mode.
>>>>>>
>>>>>> Bug link:
>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8005602
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~mgerdin/8005602/webrev.4/
>>>>>>
>>>>>>
>>>>>> The core change in this patch is the inserted call in sweep():
>>>>>> 6101
>>>>>> 6102   if (should_unload_classes()) {
>>>>>> 6103     ClassLoaderDataGraph::purge();
>>>>>> 6104   }
>>>>>> 6105
>>>>>>
>>>>>> This is needed because even though we claim to perform "class
>>>>>> unloading"
>>>>>> in the final remark phase we can't deallocate the memory for classes
>>>>>> until after we've performed the sweep phase since the sweeper 
>>>>>> needs to
>>>>>> find the size of objects even though they and their class is not
>>>>>> considered live anymore.
>>>>>>
>>>>>> The rest of the changes in concurrentMarkSweepGeneration.cpp only
>>>>>> relate
>>>>>> to logging information about released metaspace memory and cms gen
>>>>>> occupancy to make it easier to analyze what's happening. An 
>>>>>> example of
>>>>>> this new logging output is:
>>>>>>
>>>>>> 134.069: [CMS-concurrent-sweep-start]
>>>>>> 134.073: [CMS-concurrent-sweep: 0.004/0.004 secs] [Times: user=0.00
>>>>>> sys=0.02, real=0.01 secs]
>>>>>> 134.164: [CMS-resizing: [CMS: 532K->382K(63488K)], [Metaspace:
>>>>>> 277487K->133228K(370885K/412976K)] ]
>>>>>> 134.166: [CMS-concurrent-reset-start]
>>>>>> 134.179: [CMS-concurrent-reset: 0.014/0.014 secs] [Times: user=0.02
>>>>>> sys=0.00, real=0.02 secs]
>>>>>>
>>>>>> The change in genCollectedHeap.cpp is needed to avoid artificially
>>>>>> inflating the size of the Metaspace memory, since memory is 
>>>>>> considered
>>>>>> "used" until purge() has been called. By calling purge() before
>>>>>> compute_new_size() (as the other collectors do) we use the correct
>>>>>> figures.
>>>>>>
>>>>>> The disabled assert in metaspace.cpp is faulty because a thread 
>>>>>> may be
>>>>>> allocating classes and expanding the metaspace memory while we 
>>>>>> are in
>>>>>> compute_new_size, I've verified that this assert can in fact fail 
>>>>>> on a
>>>>>> build without these changes.
>>>>>> The change in ~SpaceManager is because of lock order restrictions,
>>>>>> sum_capacity_in_chunks_in_use takes the Metaspace lock and would 
>>>>>> do it
>>>>>> out of order with regards to the expand_lock.
>>>>>>
>>>>>> I've tested these changes primarily by running the
>>>>>> ParallelClassLoading
>>>>>> tests with a small young gen size to enable regular class unloading
>>>>>> cycles.
>>>>>>
>>>>>> Thanks
>>>>>> /Mikael