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

[Stefan Johansson]

Thanks for looking at this Thomas,

On 2016-09-09 13:43, Thomas Schatzl wrote:
> Hi Stefan,
>
> On Fri, 2016-09-02 at 13:10 +0200, Stefan Johansson wrote:
>> Hi,
>>
>> Please review this fix for:
>> https://bugs.openjdk.java.net/browse/JDK-8114823
>>
>> Webrev:
>> http://cr.openjdk.java.net/~sjohanss/8114823/hotspot.00/
>>
>> Summary:
>> For the G1 full GC there was no explicit code to handle when class
>> unloading was disabled. It worked for some cases because when
>> ClassUnloading is false classes in the system dictionary were kept
>> alive  and never unloaded. But some types of classes, for example
>> classes defined without a protection domain, were not kept alive.
>> This fix changes the way we process roots in phase one of the full GC
>> to include all class roots and also do the code cache scanning needed
>> when classes aren't unloaded.
>>
>> The fix adds similar argument parsing code as were previously added
>> for CMS, so I moved them together instead of having to separate
>> checks that do the same thing.
>>
>> Testing:
>> * RBT with -XX:-ClassUnloading on various test sets.
>> * New regression test that previously failed now passes.
>> * Verified that a few crashes I saw when running -ClassUnloading
>> went
>> away with the fix.
>    looks good except for one issue: The implementations for
> process_all_roots() and process_all_roots_no_string_table() are
> basically copy&paste with the only difference that the former has this
> call to process the string table roots.
>
> While reviewing this, this made it kind of hard to see the difference,
> and I am kind of wary that this code duplication may cause issues at
> some point.
>
> I would strongly prefer if one method either called the other, or the
> change added a _private_ third method with a flag to determine whether
> string processing should be done in there.
>
> If others object, I would be good with that too, but still see this as
> unnecessary maintenance issue.
I see your point, and I agree that from a maintenance point of view it 
is better. Here's an example of how it would look:
Full: http://cr.openjdk.java.net/~sjohanss/8114823/hotspot.01/
Inc: http://cr.openjdk.java.net/~sjohanss/8114823/hotspot.00-01/

Let's see what other reviewers think.

Thanks,
Stefan

> Thanks,
>    Thomas
>