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

Eric Caspole eric.caspole at oracle.com
Mon Mar 16 18:28:49 UTC 2015

This isn't something introduced with this changeset but I found the names _n_workers_done_with_threads/mark_worker_done_with_threads() pretty confusing. I see that it means "Threads::oops_do has happened by now", but if you look at it out of context it is hard to tell if "thread" is referring to the java threads being gc-ed or the gc worker threads.

May I lobby for a better name for that, maybe mark_worker_processed_java_threads() etc, or at least a comment near the declaration? 

----- Original Message -----
From: bengt.rutisson at oracle.com
To: mikael.gerdin at oracle.com, hotspot-gc-dev at openjdk.java.net
Sent: Monday, March 16, 2015 11:36:37 AM GMT -05:00 US/Canada Eastern
Subject: Re: RFR (M) 8075210: Refactor strong root processing in order to allow G1 to evolve separately from GenCollectedHeap

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

Thanks for fixing these things!

Looks good.


> /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