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

[Thomas Schatzl]

Hi Mikael,

On Fri, 2015-11-20 at 17:10 +0100, Mikael Gerdin wrote:
> Hi Thomas,
> 
> On 2015-11-19 11:56, Thomas Schatzl wrote:
> > 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
> 
> It looks like webrev.3 is not the complete set of changes.
> 
> The changes to update_ihop_statistics are in line with what I imagined.

New webrevs after Erik's suggestions (some minor changes):

http://cr.openjdk.java.net/~tschatzl/8136681/webrev.3_to_4
http://cr.openjdk.java.net/~tschatzl/8136681/webrev.4

I fixed webrev.3 too.

Thanks,
  Thomas