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

[Derek White]

Hi Jesper,

This looks good!

  - Derek

On 6/2/16 9:23 AM, Jesper Wilhelmsson wrote:
> Hi,
>
> I finally got back to looking at this.
>
> The last change had an issue as noted by Derek below, and by StefanK 
> offline, we didn't handle nmethod scanning in the same way in the 
> different collectors. Both approaches are most likely OK, but we 
> should use the same approach in all GCs.
>
> The way we did it in G1, to treat all nmethods as strong roots, is the 
> way we chose to implement right now. This is a safer/smaller step than 
> to perform nmethod unloading even though class unloading is disabled. 
> In the future we may want to perform nmethod unloading as is done in 
> the other approach, but this should be done in a separate change.
>
> A new webrev is available here:
>
> http://cr.openjdk.java.net/~jwilhelm/8114823/webrev.03/
>
> I have run this through JPRT and GCBasher with all collectors and 
> ClassUnloading set to false. It seems to hold together.
>
> Thanks,
> /Jesper
>
>
> Den 2/5/16 kl. 17:46, skrev Derek White:
>> Hi Jesper,
>>
>> On 5/2/16 11:22 AM, Jesper Wilhelmsson wrote:
>>> Hi,
>>>
>>> A new version of the patch is now available:
>>>
>>> http://cr.openjdk.java.net/~jwilhelm/8114823/webrev.02/
>>>
>>> Based on comments from Mikael, Derek, and Stefan K I have made the 
>>> following
>>> changes:
>>>
>>> * CMSClassUnloadingEnabled is removed for real and made an alias of
>>> ClassUnloadingWithConcurrentMark. This removes the questionable code 
>>> when
>>> initializing the flags.
>>>
>>> * A new call to unload nmethods if class unloading is disabled in 
>>> psMarkSweep,
>>> psParallelCompact, and genMarkSweep. It turned out that in some 
>>> cases the call
>>> to unload nmethods is needed even if we do not perform class unloading.
>> My only question is why don't we need to do that for G1 also? We used 
>> to, but
>> change at g1MarkSweep.cpp:Line 165 means we don't anymore?
>>
>> The rest looks good to me.
>>
>>  - Derek
>>>
>>> Thanks,
>>> /Jesper
>>>
>>>
>>> Den 31/3/16 kl. 15:51, skrev Jesper Wilhelmsson:
>>>> 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/
>>>>
>>>> Thanks,
>>>> /Jesper
>>