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

[Jesper Wilhelmsson]

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
>