Bugfix on CMSClassUnloadingEnabled used with CMSClassUnloadingMaxInterval > 0
Jon Masamitsu
jon.masamitsu at oracle.com
Wed Oct 2 14:34:59 UTC 2013
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.
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