RFR: JDK-8114823 - G1 doesn't honor request to disable class unloading

[Mikael Gerdin]

Hi,

On 2016-03-31 17:29, Jesper Wilhelmsson wrote:
> Hi Mikael,
>
> Den 31/3/16 kl. 17:04, skrev Mikael Gerdin:
>> Hi Jesper,
>>
>> On 2016-03-31 16:50, Jesper Wilhelmsson wrote:
>>> Hi Mikael,
>>>
>>> Thanks for looking at this!
>>>
>>> I agree that it would look better (and probably be more semantically
>>> correct) to disable CMSClassUnloadingEnabled as well. It will look weird
>>> to users who are used to look at this flag if it says true but class
>>> unloading still doesn't happen. I also updated the description of
>>> CMSClassUnloadingEnabled to say that it is equivalent to
>>> ClassUnloadingWithConcurrentMark.
>>>
>>> New webrev:
>>> http://cr.openjdk.java.net/~jwilhelm/8114823/webrev.01/
>>
>> One final comment:
>> now it feels a bit strange that if -ClassUnloading is set then
>> CMSClassUnloading
>> will not have its default value and we will end up inside the second if
>> statement always.
>>
>> Perhaps it would be better to reorder the ifs so that we first check for
>> CMSClassUnloadingEnabled and adjust ClassUnloadingWithConcurrentMark
>> properly,
>> then if class unloading is completely disabled then just make them all
>> false.
>>
>> Does that make sense to you?
>
> I thought about this as well, but the reason for having it in the order
> it is now is if someone would set -XX:-ClassUnloading
> -XX:+CMSClassUnloadingEnabled which is a valid combination. In this
> case, if we change the order, we would disable CMSClassUnloadingEnabled.
> Of course we could add checks to see if it is set on the command line
> before disabling it, but that would in my opinion add a lot of code just
> to avoid setting ClassUnloadingWithConcurrentMark to false twice.
>
> If you have a strong opinion about this I can try to come up with
> something better, but I would prefer to keep it the way it is.

I see your point.
/Mikael

> /Jesper
>
>>
>> /Mikael
>>
>>>
>>> Thanks,
>>> /Jesper
>>>
>>>
>>> Den 31/3/16 kl. 16:21, skrev Mikael Gerdin:
>>>> Hi Jesper,
>>>>
>>>> On 2016-03-31 15:51, Jesper Wilhelmsson wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this change to make the GCs disable class unloading if
>>>>> disabled on the command line.
>>>>>
>>>>> There are a few parts in this change:
>>>>>
>>>>> 1. The flag ClassUnloadingWithConcurrentMark was not set to false if
>>>>> ClassUnloading was disabled on the command line. This is now done.
>>>>>
>>>>> 2. CMS was using CMSClassUnloadingEnabled instead of the more generic
>>>>> ClassUnloadingWithConcurrentMark. I changed so that
>>>>> ClassUnloadingWithConcurrentMark is used all over and
>>>>> CMSClassUnloadingEnabled is only used as an alias when initializing
>>>>> the
>>>>> arguments. At some point it would be nice to deprecate
>>>>> CMSClassUnloadingEnabled but it is a fairly well known flag so that's
>>>>> not part of this change.
>>>>>
>>>>> 3. CMS, Serial, and PS did correctly disable class unloading when told
>>>>> so, but the outermost code witch included some logging was still
>>>>> enabled
>>>>> making class unloading output present in the log file. I changed so
>>>>> that
>>>>> this code is also disabled.
>>>>>
>>>>> 4. G1 did not disable class unloading during full GCs at all. This is
>>>>> now done.
>>>>>
>>>>> Testing: I used SecureDBBTest.java as suggested in the bug while
>>>>> fixing
>>>>> to verify that class unloading was happening before the fix and not
>>>>> after. I also ran it through JPRT.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8114823
>>>>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8114823/webrev.00/
>>>>
>>>> I think this change seems good overall.
>>>> I have one small concern regarding CMSClassUnloadingEnabled, and that
>>>> is that if
>>>> I set -ClassUnloading then CMSClassUnloadingEnabled will still be
>>>> true, would it
>>>> make sense to still disabled it in the !ClassUnloading block even if
>>>> nobody's
>>>> supposed to look at CMSClassUnloadingEnabled?
>>>>
>>>> Another approach could be to just add CMSClassUnloadingEnabled as an
>>>> alias for
>>>> ClassUnloadingWithConcurrentMark but I haven't researched exactly
>>>> how the
>>>> aliased_jvm_flags table works.
>>>>
>>>> /Mikael
>>>>
>>>>>
>>>>> Thanks,
>>>>> /Jesper