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

[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