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

Mikael Gerdin mikael.gerdin at oracle.com
Wed Mar 16 14:13:10 UTC 2016

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

}

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

More information about the hotspot-gc-dev mailing list