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

[Jesper Wilhelmsson]

Hi Derek!

Thanks for looking at this! I'll make the flag an alias instead. A new webrev 
will be presented once it's done.
/Jesper


Den 20/4/16 kl. 23:45, skrev Derek White:
> Hi Jesper,
>
> On 3/31/16 10:21 AM, Mikael Gerdin wrote:
>> 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.
>
> I researched this when I wrote it, now I have to remember how to do it :-)
>
> OK, just add this to the end of the "aliased_jvm_flags" table around line 407.
>    { "CMSClassUnloadingEnabled", "ClassUnloadingWithConcurrentMark" },
>
> This creates an aliased, but not deprecated option. Early in argument
> processing, if "CMSClassUnloadingEnabled" is passed in gets replaced by
> "ClassUnloadingWithConcurrentMark".***
>
> Oh, also delete the declaration of CMSClassUnloadingEnabled in globals.hpp.
>
> Then you can kill code trying to synchronize the CMSClassUnloadingEnabled and
> ClassUnloadingWithConcurrentMark flags.
>
> Also, all the rest of the code (the important parts) looks good!
>
> *** Side issues:
> 1) Changing a flag into an alias should only get you into trouble if there's a
> test that dumps the options values and looks for the alias name (it won't be
> there). I didn't see any tests that look for "CMSClassUnloadingEnabled".
>
> 2) Over in /deploy, there's a file secureArgs.c that mentions a bunch of flags,
> many of which don't exist anymore or have been replaced by UL. It also mentions
> CMSClassUnloadingEnabled. I suggest making the change above, then file a bug in
> "deploy" to have someone clean up secureArgs.c.
>
>   - Derek
>>
>> /Mikael
>>
>>>
>>> Thanks,
>>> /Jesper
>