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

Mikael Gerdin mikael.gerdin at oracle.com
Tue May 26 08:54:24 UTC 2015

Hi Derek,
Sorry for the delay, I've been away on vacation and been pretty busy 
after that.

On 2015-05-23 00:39, Derek White wrote:
> 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.

The include guard in g1CollectorState.hpp does not follow the convention.

Is there any reason for keeping G1CollectedHeap::full_collection() 
instead of having its callers call collector_state()->full_collection()?

It looks like you have a bunch of whitespace changes in g1CollectedHeap.cpp:

  856       result = do_collection_pause(word_size, gc_count_before, 
  857           GCCause::_g1_inc_collection_pause);

  982       result = do_collection_pause(word_size, gc_count_before, 
  983           GCCause::_g1_humongous_allocation);

(and a bunch more)
You might want to double check your changes by looking at the diff, 
webrev does not show whitespace changes by default.

In g1CollectedHeap.hpp you have some trailing whitespace on a couple of 
empty lines.

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

More information about the hotspot-gc-dev mailing list