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

[Mikael Gerdin]

Thomas,

On 2015-11-23 11:16, Thomas Schatzl wrote:
> 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

Since you decided to remove "size_t rs_size = ..." from 
record_collection_pause_end you get to remove the assert on the line 
above since it refers to the subtraction you just removed.
And now it appears that all uses of 
_cur_collection_pause_used_regions_at_start are gone and you can remove 
it altogether!

I don't need to see a webrev with the additional removals.

I think this is ready to go in now, Reviewed.

/Mikael

>
> I fixed webrev.3 too.
>
> Thanks,
>    Thomas
>
>