Bugfix on CMSClassUnloadingEnabled used with CMSClassUnloadingMaxInterval > 0

[Mikael Gerdin]

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:

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