Bugfix on CMSClassUnloadingEnabled used with CMSClassUnloadingMaxInterval > 0

[Mikael Gerdin]

On 09/30/2013 08:29 PM, Jon Masamitsu wrote:
>
> On 9/30/13 8:33 AM, Mikael Gerdin wrote:
>> Jungwoo,
>>
>> On 09/25/2013 12:36 AM, Jungwoo Ha wrote:
>>> Mikael,
>>>
>>> I updated the patch as you mentioned.
>>> There are no "set"_root_scanning_option, so I added that.
>>> http://cr.openjdk.java.net/~rasbold/8024954/webrev/
>>
>> Why do you check CMSClassUnloadingEnabled before checking
>> should_unload_classes()?
>> This will cause the code for
>> ExplicitGCInvokesConcurrentAndUnloadsClasses to break, since it
>> doesn't depend on CMSClassUnloading being set.
>>
>> I think you could just move
>> "set_root_scanning_option(SharedHeap::SO_SystemClasses);" into the
>> existing should_unload_classes() check.
>>
>> Upon closer inspection I think that
>> "set_root_scanning_option(SO_AllClasses|SO_Strings|SO_CodeCache);"
>> in the case of !should_unload_classes() is incorrect since we should
>> be unloading strings every cycle regardless of the class unloading state.
>> Maybe it would be better to keep part of your original suggestion and
>> doing:
>>
>
> Mikael,
>
> Could you explain your suggestion of using set_root_scanning_option()?
> _root_scanning_options is part of CMSCollector and there's only one
> CMSCollector
> (right?) so whatever is in _root_scanning_options persists from
> collection to collection.
> I don't know that there are some bits set in _root_scanning_options that
> should
> be preserved but it's possible so resetting _root_scanning_options with
> set_root_scanning_option() could be losing bits.

Jon,

I'm actually pretty confused by the code which adds and removes 
SO_Strings and SO_CodeCache from the CMSCollector's root_scanning_options.
I don't understand why they should ever be roots for CMS, the only 
ScanOptions which make sense to me are SO_SystemClasses or 
SO_AllClasses, depending on wether we are going to unload classes this 
particular CMS cycle.

Since I believe that these are the only two reasonable values I think 
that the add/remove logic is confusing to readers of the code if there 
aren't any other values that need to be preserved.

I think that this ScanOption mess is somehow related to perm gen 
verification which has (obviously) been removed from 8, and the fact 
that interned strings were in the perm gen before 7, so if we were not 
going to sweep perm gen we would just treat the StringTable as roots.

As soon as (should_unload_classes() != CMSClassUnloadingEnabled) (for 
any reason) we will run into the bug we are trying to fix here.

/Mikael

>
> Jon
>
>> if (should_unload_classes()) {
>>   set_root_scanning_option(SO_SystemClasses);
>>   set_verifying(should_verify);
>>   return;
>> } else {
>>   remove_root_scanning_option(SO_SystemClasses);
>>   add_root_scanning_option(SO_AllClasses);
>> }
>>
>>
>> The whole chunk of code below
>> assert(!should_unload_classes()) seems suspect to me, it adds the
>> strings to the root set if verification is enabled which seems to
>> suggest that if verification is enabled CMS will never unlink interned
>> strings. I've been digging around a bit in the history and I think the
>> code is related to perm gen verification, which does not exist in 8
>> any more, so we may be able to get rid of it. I want to understand the
>> reason for adding and removing SO_Strings and SO_CodeCache, because
>> doing that does not make sense to me in the 8 codebase.
>>
>> /Mikael
>>>
>>> Jungwoo
>>>
>>>
>>> On Thu, Sep 19, 2013 at 4:48 AM, Mikael Gerdin <mikael.gerdin at oracle.com
>>> <mailto:mikael.gerdin at oracle.com>> wrote:
>>>
>>>     Hi Jungwoo,
>>>
>>>
>>>     On 2013-09-17 01:14, Jungwoo Ha wrote:
>>>
>>>         Hi,
>>>
>>>         We found that hotspot crashes when CMSClassUnloadingEnabled is
>>>         true, and
>>>         CMSClassUnloadingMaxInterval > 0.
>>>         This is on hotspot24 and u40.
>>>         This is easily reproducible with DaCapo tradesoap benchmark with
>>>         heapsize around 200MB.
>>>
>>>         The reason for the crash is that CMS sets the root set when
>>>         CMSClassUnloadingEnabled is on during the constructor phase
>>> assuming
>>>         that every CMS cycle will unload the class.
>>>         However, when CMSClassUnloadingMaxInterval > 0, CMS may not
>>> unload
>>>         classes ended up crashing.
>>>         I think this is apparently a bug, and I attach the fix.
>>>         Please take a look at the attached patch.
>>>         My changes are resetting the root scanning option on every CMS
>>>         cycle in
>>>         setup_cms_unloading_and___verification_state() if
>>>         CMSCLassUnloadingEnabled
>>>         is on.
>>>
>>>         Please take a look and let us know how to proceed.
>>>
>>>
>>>     Jon filed a bug for this:
>>>     https://bugs.openjdk.java.net/__browse/JDK-8024954
>>>     <https://bugs.openjdk.java.net/browse/JDK-8024954>
>>>
>>>     I think the code change fixes the bug but I'd like it better if you
>>>     changed the code to always set the requested ScanOption values in
>>>     setup_cms_unloading_and___verification_state
>>>
>>>     something like:
>>>     if (should_unload_classes()) {
>>>     set_root_scanning_option(SO___SystemClasses);
>>>     } else {
>>> set_root_scanning_option(SO___AllClasses|SO_Strings|SO___CodeCache);
>>>     }
>>>
>>>     I also believe that your check for CMSClassUnloadingEnabled can
>>>     cause a bug similar to the one you are trying to fix if
>>>     ExplicitGCInvokesConcurrentAnd__UnloadsClasses would be enabled.
>>>     should_unload_classes() should be sufficient to determine which
>>>     ScanOptions to set for a particular cms cycle.
>>>
>>>     Please send a patch/webrev based on the latest jdk8 sources
>>>     http://hg.openjdk.java.net/__hsx/hotspot-gc/hotspot
>>>     <http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot>
>>>     Since the function is also messing around with the verification
>>>     flags, make sure to run some tests on your changes with verification
>>>     enabled.
>>>
>>>     /Mikael
>>>
>>>
>>>         Thanks,
>>>         Jungwoo
>>>
>>>
>>
>