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

[Jon Masamitsu]

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.

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