RFR (M) 8075210: Refactor strong root processing in order to allow G1 to evolve separately from GenCollectedHeap

Mikael Gerdin mikael.gerdin at oracle.com
Mon Mar 16 15:22:11 UTC 2015


Hi Bengt,

On 2015-03-16 15:55, Bengt Rutisson wrote:
>
> Hi Mikael,
>
> Nice cleanup! Thanks for splitting the webrev up into several parts.
> Made it much easier to review.
>
> Looks good to me.
>
> One minor thing is that the comment at the top of sharedHeap.hpp uses
> G1CollectedHeap::g1_process_roots() as an example for how to use a
> FlexibleWorkGang. Maybe we should update that comment.

I updated the comment.

>
> Also, in g1CollectedHeap.cpp, there is this code:
>
> 3111     G1RootProcessor root_processor(this, /* trace_metadata */ false);
>
> I'm more used to see the comment after the value then before. I.e:
>
> G1RootProcessor root_processor(this, false /* trace_metadata */);

Ok, I handn't noticed the distinction. Fixed.


The comment updates are in an incremental webrev at:
http://cr.openjdk.java.net/~mgerdin/8075210/comments/webrev/

/Mikael

>
> Thanks,
> Bengt
>
>
> On 2015-03-16 14:37, Mikael Gerdin wrote:
>> Hi all!
>>
>> Currently SharedHeap::process_strong_roots is called both by the
>> GenCollectedHeap based collectors (CMS, Serial) and G1.
>>
>> Since G1 needs special cases for several pieces of the root processing
>> SharedHeap needs to allow for that while attempting to maintain some
>> layer of abstraction. This makes the SharedHeap code unnecessarily
>> complex and makes it very difficult to reason about which combination
>> of parameters are valid and possible.
>>
>> As a first step to improve the code I suggest that we copy the root
>> processing code to a separate class for G1 and move the SharedHeap
>> root processing to GenCollectedHeap. For now I think it's worth it to
>> introduce slightly more duplication of the root processing code in
>> order to reduce the complexity of the shared code path.
>>
>> Overall the change is consists of
>>  15 files changed, 597 insertions(+), 551 deletions(-)
>> Where the insertions include two new copies of the GPL license header.
>>
>> An additional goal for this change is to make it easier to get rid of
>> SharedHeap altogether at some future point, there's really no point in
>> having a shared abstract base class between G1 and GenCollectedHeap.
>>
>> If we want to we can also modify G1RootProcessor at some future point
>> to allow it to be used for all root processing, including ParallelGC
>> and JVMTI. For example the generic root processor could be implemented
>> as some kind of class template, where function objects are passed as
>> template parameters to control claiming of tasks and which roots to
>> visit.
>>
>> In order to make it easier to review I've split up the change into 3
>> phases:
>> 1. Copy SharedHeap::process_roots and
>> G1CollectedHeap::g1_process_roots to G1RootProcessor::process_roots
>> and convert G1's evacuation code to use G1RootProcessor.
>> 2. Convert the rest of G1 (verification and G1MarkSweep) to use
>> G1RootProcessor. Split G1RootProcessor::process_roots in order to
>> allow for the different needs of different callers.
>> 3. Move SharedHeap::process_roots to GenCollectedHeap since it's now
>> the only caller of the code. Get rid of the ScanOption operator| since
>> we never or them any longer.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8075210
>>
>> Webrevs:
>> http://cr.openjdk.java.net/~mgerdin/8075210/phase-1/webrev
>> http://cr.openjdk.java.net/~mgerdin/8075210/phase-2/webrev
>> http://cr.openjdk.java.net/~mgerdin/8075210/phase-3/webrev
>>
>> Full webrev:
>> http://cr.openjdk.java.net/~mgerdin/8075210/full/webrev
>>
>> Testing:
>> JPRT
>>
>> /Mikael
>



More information about the hotspot-gc-dev mailing list