RFR (M) 8075210: Refactor strong root processing in order to allow G1 to evolve separately from GenCollectedHeap
bengt.rutisson at oracle.com
Mon Mar 16 15:31:17 UTC 2015
On 2015-03-16 16:22, Mikael Gerdin wrote:
> 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 */
>> 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:
Thanks for fixing these things!
>> 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
>>> In order to make it easier to review I've split up the change into 3
>>> 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.
>>> Full webrev:
More information about the hotspot-gc-dev