Bugfix on CMSClassUnloadingEnabled used with CMSClassUnloadingMaxInterval > 0
Mikael Gerdin
mikael.gerdin at oracle.com
Fri Oct 4 13:20:01 UTC 2013
Jon,
On 10/02/2013 04:34 PM, Jon Masamitsu wrote:
> Mikael,
>
> I heard back from Ramki on the setting of the scanning options and it
> likely is not what I thought it was. He commented that your suggestion
> about SO_SystemClasses or SO_AllClasses being the reasonable
> possibilities sounded right. But I'm a bit hesitant to change to using
> a set_root_scanning_option() (discarding the persistent state in
> _root_scanning_options) without having a very clear understanding
> about how we got to the current code.
>
> For Jungwoo's change I'd like to continue to use the add / remove
> paradigm and investigate the set_root_scanning_option()
> separately.
Fair enough. Let's keep the add/remove code for now.
Hopefully we'll have the time to clean this up at some point in the
future. I suspect that a lot of this complexity can be removed
completely now that perm gen is gone. CMS desperately needs any decrease
in complexity it can get :)
/Mikael
>
> Jon
>
> On 10/1/2013 10:13 PM, Jon Masamitsu wrote:
>>
>> On 10/1/2013 6:51 AM, Mikael Gerdin wrote:
>>> 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.
>>
>> From looking at the SO_CodeCache ScanningOption this may be related to
>> a class unloading bug that was fixed some time during jdk7 development.
>> C2 keeps some profiling information at call sites which include
>> pointers to
>> methods. The profiling information was keeping classes alive. The
>> desired behavior was that the pointers to methods in the profiling
>> information would act as weak reference. As I recall the fix for all the
>> collectors except CMS was straight forward but the CMS fix was
>> rather complicated and may have used the ScanningOption. I'll
>> ping Ramki on this to see if he remembers anything about it.
>>
>> Jon
>>
>>>
>>> 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-gc-dev
mailing list