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

[Mikael Gerdin]

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

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