RFR (XXS): 8049051: Use of during_initial_mark_pause() in G1CollectorPolicy::record_collection_pause_end() prevents use of seperate object copy time prediction during marking

[Bengt Rutisson]

Hi Thomas,

Looks good!

Bengt


On 2014-07-10 11:08, Thomas Schatzl wrote:
> Hi Bengt,
>
>    thanks for the review. See below for my comments.
>
> On Tue, 2014-07-08 at 08:49 +0200, Bengt Rutisson wrote:
>> Hi Thomas,
>>
>> On 2014-07-02 14:33, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     some apparent bug in the G1 pause time prediction logic: when ending
>>> an initial mark pause, G1 is supposed to switch to different predictors
>>> for the object copy time.
>>>
>>> Since the code that sets the flag to indicate to the prediction that the
>>> other predictor should be used for object copy time prediction wrongly
>>> reuses during_initial_mark_pause() that has been unconditionally set to
>>> false before, G1 never switches predictors.
>>>
>>> The fix is to use the backup ("last_pause_included_initial_mark") value
>>> for during_initial_mark_pause() already used in several places in that
>>> method.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8049051
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8049051/webrev/
>> The fix looks good.
>>
>> One question.
>>
>> Why do we need these local variables?
>>
>>     bool new_in_marking_window = _in_marking_window;
>>     bool new_in_marking_window_im = false;
>>
>>
>> They are only used in one place further down to set the corresponding
>> instance variables:
>>
>>     _in_marking_window = new_in_marking_window;
>>     _in_marking_window_im = new_in_marking_window_im;
>>
>>
>> Can't we just move the if (last_pause_included_initial_mark)  check to
>> that place and skip the local variables?
> Yes, we can :)
>
> New webrev at
> http://cr.openjdk.java.net/~tschatzl/8049051/webrev.1/
>
> Testing:
> JPRT
>
> Thanks,
>    Thomas
>
>