Request for Review 8024483 - assertion failure: (!mirror_alive || loader_alive) failed AND 8024954: CMS: CMSClassUnloadingMaxInterval is not implemented correctly

[Jon Masamitsu]

On 11/4/2013 1:02 AM, Bengt Rutisson wrote:
>
> Hi Jon,
>
> On 2013-11-02 01:02, Jon Masamitsu wrote:
>> 8024483 - assertion failure: (!mirror_alive || loader_alive) failed
>>
>> The default value of _should_unload_classes in CMSCollector is false.
>> The default value of CMSClassUnloadingEnabled is true (changed in jdk8)
>> and is used to set the root scanning options.  This inconsistency 
>> leads to
>> dead class loaders without unloading classes.
>>
>> The fix for 8024954 fixes (or works around) this inconsistency by 
>> resetting
>> the root scanning options based on _should_unload_classes.
>>
>> Additionally there is a missing call to update_should_unload_classes() in
>> the CMS foreground collector.  Adding the call allows the test for
>> CMSClassUnloadingEnabled  to pass but may not be a complete fix.
>>
>> http://cr.openjdk.java.net/~jmasa/8024483/webrev.00/
>
> Looks good.
>
>>
>>
>> 8024954: CMS: CMSClassUnloadingMaxInterval is not implemented correctly
>>
>> For 8024954 the classes used as roots was being set at initialization 
>> based on
>> the value of CMSClassUnloadingEnabled. For a value of 
>> CMSClassUnloadingMaxInterval
>> greater than 1, the roots need to be set dynamically
>> (done in CMSCollector::setup_cms_unloading_and_verification_state 
>> with this fix).
>>
>> http://cr.openjdk.java.net/~jmasa/8024954/webrev.01/ 
>
>
> Looks good. One minor question. In CMSCollector::CMSCollector() you 
> left this line:
>
>  792   add_root_scanning_option(SharedHeap::SO_SystemClasses);
>
> But in CMSCollector::setup_cms_unloading_and_verification_state() we 
> now always set up the state correctly to be either SO_AllClasses or 
> SO_SystemClasses. So, do we need that initilization in 
> CMSCollector::CMSCollector()?

I tested both with and without it and it is not needed as far as I can tell.
I left it in because I know that SO_None is always wrong (as set by the
constructor).  I didn't want to try and figure out what the best default
was based on the other flags because that was the source of this bug.
If you think leaving it initialized to SO_None makes the code more readable,
I can remove it.

Jon

>
> Thanks,
> Bengt

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20131104/14990219/attachment.htm>