[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR 7097567: G1: abstract and encapsulate collector phases and transitions between them
From:       Derek White <derek.white () oracle ! com>
Date:       2015-05-26 19:09:14
Message-ID: 5564C4DA.3080009 () oracle ! com
[Download RAW message or body]

Thanks Mikael!

New webrev: http://cr.openjdk.java.net/~drwhite/7097567/webrev.05/

Comments below...

On 5/26/15 4:54 AM, Mikael Gerdin wrote:
> 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.

OK
> Is there any reason for keeping G1CollectedHeap::full_collection() 
> instead of having its callers call collector_state()->full_collection()?
No reason that makes sense now.
> It looks like you have a bunch of whitespace changes in 
> g1CollectedHeap.cpp:
>
>  856       result = do_collection_pause(word_size, gc_count_before, 
> &succeeded,
>  857           GCCause::_g1_inc_collection_pause);
>
>  982       result = do_collection_pause(word_size, gc_count_before, 
> &succeeded,
>  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.

I'm not sure where those came from, but there was a difficult merge. I 
changed my IDE setting to pay attention to whitespace when doing diffs.
> In g1CollectedHeap.hpp you have some trailing whitespace on a couple 
> of empty lines.

OK.
> /Mikael

Thanks again!

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

[prev in list] [next in list] [prev in thread] [next in thread]