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

[Mikael Gerdin]

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.

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.

/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