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

[Mikael Gerdin]

On 2013-03-12 13:45, Mikael Gerdin wrote:
> 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.

For reference, the changes with the logging reverted are:
http://cr.openjdk.java.net/~mgerdin/8005602/webrev.5/

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