Bugfix on CMSClassUnloadingEnabled used with CMSClassUnloadingMaxInterval > 0
Jungwoo Ha
jwha at google.com
Wed Oct 9 00:18:13 UTC 2013
It sounds to me that I don't need to change the webrev I sent you. Is that
right? --Jungwoo
On Fri, Oct 4, 2013 at 6:20 AM, Mikael Gerdin <mikael.gerdin at oracle.com>wrote:
> 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/<http://cr.openjdk.java.net/~rasbold/8024954/webrev/>
>>>>>>>
>>>>>>
>>>>>> Why do you check CMSClassUnloadingEnabled before checking
>>>>>> should_unload_classes()?
>>>>>> This will cause the code for
>>>>>> ExplicitGCInvokesConcurrentAnd**UnloadsClasses 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 <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>
>>>>>>> <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>
>>>>>>> <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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20131008/3fcbf956/attachment.htm>
More information about the hotspot-gc-dev
mailing list