RFR(S): JDK-8139868: CMSScavengeBeforeRemark broken after JDK-8134953

[Bengt Rutisson]

On 2015-10-20 10:27, Stefan Johansson wrote:
> Hi Bengt,
>
> Change looks good.

Thanks, Stefan!

Bengt

>
> Thanks,
> Stefan
>
> On 2015-10-19 13:07, Bengt Rutisson wrote:
>>
>> Hi again,
>>
>> A small update on this.
>>
>> The test that is included in the review request only tests that the 
>> VM does not crash when run with CMSScavengeBeforeRemark. This is 
>> enough to verify the current fix. The test fails (asserts) without 
>> the fix and passes with it.
>>
>> But the test does not verify the actual feature that 
>> CMSScavengeBeforeRemark enables. That is, to do a young GC before the 
>> remark phase in CMS.
>>
>> I discussed this briefly with Dima. We agreed to file an RFE to 
>> improve the test as a separate task. I filed JDK-8139877 to track 
>> that work.
>>
>> Improve TestCMSScavengeBeforeRemark
>> https://bugs.openjdk.java.net/browse/JDK-8139877
>>
>> Thanks,
>> Bengt
>>
>> On 2015-10-19 10:24, Bengt Rutisson wrote:
>>>
>>> Hi everyone,
>>>
>>> Could I have a couple of reviews for this fix for a GC id issue?
>>>
>>> http://cr.openjdk.java.net/~brutisso/8139868/webrev.00/index.html
>>> https://bugs.openjdk.java.net/browse/JDK-8139868
>>>
>>> From the CR:
>>>
>>> Running with -XX:+CMSScavengeBeforeRemark will do an extra call to 
>>> GenCollectedHeap::do_collection() inside 
>>> CMSCollector::checkpointRootsFinal().
>>>
>>> Inside CMSCollector::checkpointRootsFinal() we already have a valid 
>>> GC id set up, but the call to GenCollectedHeap::do_collection() will 
>>> create a new one using a GCIdMark, which means that when it returns 
>>> the GC id is set to undefined.
>>>
>>> It is correct that GenCollectedHeap::do_collection() creates a new 
>>> GC id since we are doing one extra GC there. But it should not reset 
>>> the GC id to undefined when it is done.
>>>
>>>
>>> The fix is to use a GCIdMarkAndRestore inside 
>>> GenCollectedHeap::do_collection() instead. This will preserve the 
>>> "outer" GC id for the remark. But since the two other calls to 
>>> GenCollectedHeap::do_collection() are for distinct GCs we can not 
>>> assume that there always is an "outer" GC id to preserve. Thus, the 
>>> constructor for GCIdMarkAndRestore has to use current_raw() instead 
>>> of current() to store the previous GC id.
>>>
>>> Thanks,
>>> Bengt
>>
>