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

[Bengt Rutisson]

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.

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 */);

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