RFR: 8114823: G1 doesn't honor request to disable class unloading
stefan.johansson at oracle.com
Mon Sep 12 08:49:23 UTC 2016
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:
>> Please review this fix for:
>> 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.
>> * 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
>> 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:
Let's see what other reviewers think.
More information about the hotspot-gc-dev