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

[Mikael Gerdin]

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.

/Mikael

>
> Thanks,
>    Thomas
>
>