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? 
Thanks,
Eric

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

Bengt

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