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

[Bengt Rutisson]

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