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

[Mikael Gerdin]

Stefan,

On 2013-03-12 13:20, Stefan Karlsson wrote:
> 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.

Thanks for the review.

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

Since these changes will also conflict with Thomas' fix for 8008684 I 
will back out the logging changes and fix them in a separate CR.

/Mikael

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