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

[Mikael Gerdin]

Hi Eric,

On 2015-03-16 19:28, Eric Caspole wrote:
> 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?

That's an excellent suggestion. I'll look into it.

/Mikael

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