RFR (L) 8151711: Move G1 number sequences out of the G1 collector policy
tom.benson at oracle.com
Wed Mar 16 15:02:10 UTC 2016
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!
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.
>>> 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
>> 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.
>> New full webrev:
>> Incremental webrev:
>>> 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
>>>> 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
>>>> 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 \
>>>> 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.
More information about the hotspot-gc-dev