RFR 7097567: G1: abstract and encapsulate collector phases and transitions between them

[Derek White]

Still waiting for a review, but have a new version:

Bug: https://bugs.openjdk.java.net/browse/JDK-7097567
Webrev: http://cr.openjdk.java.net/~drwhite/7097567/webrev.04/

Changes in rev 04 vs 02:
  - Updated for GC source restructuring.
  - Removed "_in_young_gc_mode" field, which had been deleted by a bug 
fix earlier but had been mistakenly resurrected.
  - Moved yc_type() from G1CollectedHeap to G1CollectorState.

Thanks!

  - Derek

On 5/12/15 5:26 PM, Derek White wrote:
> Hi Mikael,
>
> Thanks for the review. I just got back to this. Comments inline, and 
> new webrev below...
>
> On 1/28/15 5:03 AM, Mikael Gerdin wrote:
>> Hi Derek,
>>
>> On 2015-01-27 23:41, Derek White wrote:
>>> Review request!
>>>
>>> This is a change Ramki did before he left that didn't get checked in.
>>> The basic idea is to move a bunch fields that encapsulate collector
>>> state from G1CollectorPolicy into a separate class " G1CollectorState".
>>>
>>> Thanks in advance!
>>>
>>>   - Derek
>>>
>>> /Bug/: https://bugs.openjdk.java.net/browse/JDK-7097567
>>> /Webrev/: http://cr.openjdk.java.net/~drwhite/7097567/webrev.00/
>>
>> This is just a high level review to begin with, I didn't look too 
>> carefully at all the state changes.
>>
>> I agree that it's a good idea to factor out the state changes but I'm 
>> not convinced that the CollectorState should logically be a member of 
>> the collector policy object.
>> To me it feels more like a G1 heap would have a policy object and a 
>> state object as members.
> I talked this over with Thomas and now understand the issue. I also 
> sucked up some state fields from g1CollectedHeap into g1CollectorState.
>> If the policy object needs to interact with the state object they can 
>> of course do that via a pointer to the state object from the policy.
> OK
>> Besides that I don't like the fact that the state object is declared 
>> in the g1CollectorPolicy.hpp header, I'd prefer it if you could move 
>> that to a separate new header file.
> OK
>> The change to compactibleFreeListSpace seems like an accident since 
>> it has nothing to do with G1 state changes whatsoever.
> OK
>
> - Derek
>
> ---------- NEW WEBREV ----------
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-7097567
> Webrev: http://cr.openjdk.java.net/~drwhite/7097567/webrev.02/
>
> I also added follow-on enhancement request for turning 
> G1CollectorState into a real state machine.
>
>   * JDK-8080226 <https://bugs.openjdk.java.net/browse/JDK-8080226> G1:
>     Replace collector state booleans with explicit state variable(s)
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150522/cc744818/attachment.htm>