RFR (M): 8136681: Factor out IHOP calculation from G1CollectorPolicy

[Thomas Schatzl]

Hi Mikael,

  thanks for the review. Sorry for not understanding your intention in
the first place.

On Wed, 2015-11-18 at 16:57 +0100, Mikael Gerdin wrote:
> On 2015-11-16 12:23, Thomas Schatzl wrote:
> > Hi,
> >
> > On Fri, 2015-11-13 at 16:08 +0100, Mikael Gerdin wrote:
> >> Hi Thomas,
[...]
> >
> >> 1219   double marking_to_mixed_time = -1.0;
> >> 1220   if (!collector_state()->last_gc_was_young() &&
> >> _initial_mark_to_mixed.has_result()) {
> >> 1221     marking_to_mixed_time = _initial_mark_to_mixed.last_marking_time();
> >> 1222     assert(marking_to_mixed_time > 0.0,
> >> 1223            "Initial mark to mixed time must be larger than zero but
> >> is %.3f",
> >> 1224            marking_to_mixed_time);
> >> 1225   }
> >> 1226   // Only update IHOP information on regular GCs.
> >> 1227   if (update_stats) {
> >> 1228     update_ihop_statistics(marking_to_mixed_time,
> >>
> >> Since you are only setting marking_to_mixed_time at line 1221 -1 is
> >> passed to update_ihop_statistics fairly regularly.
> >> Would it make sense to break out update_time_to_mixed and move it inside
> >> the 1220-1225 if-block?
> >
> > Done.
> 
> Hm, I was thinking more in the lines of
> 
> // Only update IHOP information on regular GCs.
> if (update_stats) {
>    if (!collector_state()->last_gc_was_young() &&
> 	_initial_mark_to_mixed.has_result()) {
>      double marking_to_mixed_time = 
> _initial_mark_to_mixed.last_marking_time();
>      assert(marking_to_mixed_time > 0.0,
>             "Initial mark to mixed time must be larger than zero but is 
> %.3f",
>             marking_to_mixed_time);
>        _ihop_control->update_marking_length(marking_to_mixed_time);
>      }
> 
>      update_ihop_statistics(app_time_ms / 1000.0,
>                             _last_old_allocated_bytes,
>                             last_unrestrained_young_length * 
> HeapRegion::GrainBytes);
>    }
>    _last_old_allocated_bytes = 0;
> 
> Otherwise a reader might think that marking_to_mixed_time is actually 
> set and updated more than once per gc cycle when it's in fact "-1" most 
> of the time and thus ignored in update_ihop_statistics.

Okay. I restructured this code as suggested, and moved it into a
separate method to not bloat
G1CollectorPolicy::record_collection_pause_end() more than required.

There were some other changes, mostly necessitated by the review from
Tom about the JFR events, which showed that a wrong value were passed to
the logging message. This caused some changes related to logging that I
thought would be best fixed here.

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8136681/webrev.2_to_3
http://cr.openjdk.java.net/~tschatzl/8136681/webrev.3

Thanks,
  Thomas