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

[Jon Masamitsu]

Mikael,

Changes look good.

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.

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