RFR (L) 8151711: Move G1 number sequences out of the G1 collector policy

[Mikael Gerdin]

Hi,

Thanks for the reviews Erik and Tom.

On 2016-03-16 16:02, Tom Benson wrote:
> Hi,
> It looks like the name change makes the indentation wrong at lines 244,
> 871, and 876 in g1CollectorPolicy.cpp,
> and at 320 in g1Analytics.cpp.  Aside from that, looks good!

I'd caught just one of them, I feel like I've stared myself blind on 
this code now...

Thanks
/Mikael

> Tom
>
> On 3/16/2016 10:36 AM, Erik Helin wrote:
>> On 2016-03-16, Mikael Gerdin wrote:
>>> Hi Tom,
>>>
>>> Thanks for the review!
>>>
>>> On 2016-03-15 20:37, Tom Benson wrote:
>>>> Hi Mikael,
>>>> This looks OK to me, with one trivial comment.  Since you're making a
>>>> new module out of it, perhaps you can make the indentation at lines
>>>> 105-112 in g1Measurements.cpp more consistent.
>>> Fixed.
>>>
>>>> If feels like maybe the existing G1Predictions.cpp/.hpp should be
>>>> folded
>>>> into this new module, now that the measurement stuff is separate from
>>>> Policy.  It's very small - most of the code is a test.  But I suppose
>>>> that could be the subject of another change.
>>> My thought with not moving G1Predictions into the new module was to
>>> allow
>>> the tunables of the predictor to be set elsewhere.
>>>
>>>> As for names, G1Analytics came to mind to imply data gathering and
>>>> prediction.
>>> I like that name!
>>>
>>> I also got some offline feedback from Erik indicating that
>>> update_recent_gc_times was actually called from full gcs as well and
>>> in that
>>> case didn't do the ratio calculations so I rewrote the code to
>>>
>>>      double interval_ms =
>>>        (end_time_sec - _analytics->last_known_gc_end_time_sec()) *
>>> 1000.0;
>>>      _analytics->update_recent_gc_times(end_time_sec, pause_time_ms);
>>>      _analytics->compute_pause_time_ratio(interval_ms, pause_time_ms);
>>>    }
>> Looks good besides a space on the wrong side of the comma:
>> +    _analytics->predict_object_copy_time_ms(bytes_to_copy
>> ,collector_state()->during_concurrent_mark());
>>
>> I don't need to re-review.
>>
>> Thanks,
>> Erik
>>
>>> New full webrev:
>>> http://cr.openjdk.java.net/~mgerdin/8151711/webrev.1/
>>> Incremental webrev:
>>> http://cr.openjdk.java.net/~mgerdin/8151711/webrev.0_to_1/
>>>
>>> Thanks
>>> /Mikael
>>>
>>>> Tom
>>>>
>>>> On 3/14/2016 4:25 AM, Mikael Gerdin wrote:
>>>>> Hi all,
>>>>>
>>>>> Currently a large part of the G1 collector policy consists of counters
>>>>> and number sequences for different measurements performed by the
>>>>> collector.
>>>>> In order to reduce the overall API surface of the G1CollectorPolicy
>>>>> class and possibly allow for different collector policy
>>>>> implementations the measurements and simple predictions based on the
>>>>> number sequences should be factored out of the policy code. The policy
>>>>> will then become more of a decision maker and not a combined data
>>>>> store and decision maker.
>>>>>
>>>>> My current working name for the new class is G1Measurements but I'm
>>>>> not overly attached to the name.
>>>>>
>>>>> I've made the new files "hg copies" of the g1CollectorPolicy files
>>>>> since a lot of methods and members are simply moved as-is to the new
>>>>> class.
>>>>> One thing I did modify was to move decisions based on the
>>>>> collector_state() out of the prediction methods and instead based the
>>>>> selection on a boolean parameter.
>>>>>
>>>>> I suggest that in order to review the changes you open up your
>>>>> favorite 3-way diff tool and view a 3-way side-by-side diff of
>>>>> g1CollectorPolicy.cpp (from my webrev),
>>>>> g1CollectorPolicy.cpp (from before my suggested changes),
>>>>> g1Measurements.cpp (from my webrev).
>>>>>
>>>>> In diffuse this would be done as:
>>>>> diffuse \
>>>>> src/share/vm/gc/g1/g1CollectorPolicy.cpp \
>>>>> -r qparent src/share/vm/gc/g1/g1CollectorPolicy.cpp \
>>>>> src/share/vm/gc/g1/g1Measurements.cpp
>>>>>
>>>>> This allows you to (hopefully) verify the moved contents.
>>>>> I've tried pretty hard to keep the code in the same order as in the
>>>>> original location.
>>>>>
>>>>> For g1Measurements.hpp I've been a bit more lax to let the header be
>>>>> nice and tidy.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8151711
>>>>> Webrev: http://cr.openjdk.java.net/~mgerdin/8151711/webrev.0/
>>>>> Testing: RBT gc testing, JPRT, Perf testing on aurora.
>>>>>
>>>>> Thanks
>>>>> /Mikael
>