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

[Bengt Rutisson]

On 2013-11-05 01:33, Jon Masamitsu wrote:
>
> 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.

I prefer to just use SO_None as default since I think that setting it to 
SharedHeap::SO_SystemClasses just gives the false impression that this 
is what we want to use. But I'm also ok with the change as it is. So, 
I'll leave it up to you to decide.

Thanks,
Bengt

>
> Jon
>
>>
>> Thanks,
>> Bengt
>

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