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

[Erik Helin]

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