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

[Jesper Wilhelmsson]

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