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

[Stefan Johansson]

Hi Bengt,

Change looks good.

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
>