Bugfix on CMSClassUnloadingEnabled used with CMSClassUnloadingMaxInterval > 0

[Jon Masamitsu]

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

> 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
>>
>>
>