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

[Eric Caspole]

Big improvement, thanks!
Eric


----- Original Message -----
From: mikael.gerdin at oracle.com
To: eric.caspole at oracle.com, bengt.rutisson at oracle.com
Cc: hotspot-gc-dev at openjdk.java.net
Sent: Tuesday, March 17, 2015 11:59:54 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

Hi Eric,

On 2015-03-17 09:02, Mikael Gerdin wrote:
> 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.

Incremental webrev incorporating the renaming:
http://cr.openjdk.java.net/~mgerdin/8075210/done_with_threads/webrev/

See my reply to Thomas' review for an updated full webrev.

/Mikael

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