RFR (L): 8064702: Remove the CMS foreground collector

[Kim Barrett]

On Nov 13, 2014, at 2:23 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> 
>> ------------------------------------------------------------------------------
>> 
>> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
>> 
>> 1248   // For debugging purposes, change the type of collection.
>> 1249   // If the rotation is not on the concurrent collection
>> 1250   // type, don't start a concurrent collection.
>> 1251   NOT_PRODUCT(
>> 1252     if (RotateCMSCollectionTypes) {
>> 1253       return _cmsGen->debug_concurrent_cycle();
>> 1254     }
>> 1255   )
>> 
>> Code was changed, but the comment looks like it needs to be updated
>> too, since there is no longer a "concurrent collection type" in the
>> previous sense.
> 
> Good catch. I updated the comment to:
> 
>  // For debugging purposes, change the type of collection.
>  // Rotate between concurrent and stop-the-world full GCs.

Looks good.

>> ------------------------------------------------------------------------------
>> 
>> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
>> 
>> 1254     if (RotateCMSCollectionTypes &&
>> 1255         (_cmsGen->debug_collection_type() !=
>> 1256           ConcurrentMarkSweepGeneration::Concurrent_collection_type)) {
>> 1257       assert(_cmsGen->debug_collection_type() !=
>> 1258         ConcurrentMarkSweepGeneration::Unknown_collection_type,
>> 1259         "Bad cms collection type");
>> 1260       return false;
>> 1261     }
>> 
>> =>
>> 
>> 1252     if (RotateCMSCollectionTypes) {
>> 1253       return _cmsGen->debug_concurrent_cycle();
>> 1254     }
>> 
>> I spent a large amount of time studying this change, because there are
>> behavioral changes here that looked suspicious.  I think I've decided
>> it's ok, but RotateCMSCollectionTypes is now badly named, and was
>> never well described and had (and still has) unclear semantics.  I
>> think I've puzzled out more or less what it means, but that required
>> reading a lot of code, and I'm still not confident that I know what
>> would happen if I used it, or why I might want to.
> 
> Right. I agree that it is unclear if this code is needed. I have never been running with RotateCMSCollectionTypes but I did not want to remove it as part of this change.
> 
> As you probably noticed the change I did was to go from three states (concurrent background, stw foreground and full serial GC) to two states (concurrent background and full serial GC). Since two states can be encoded with a bool I decided to simplify a bit a get rid of the ColletionType enum. It was easier for me to verify that I didn't break this if I could do that simplification.
> 
> I don't think this functionality is used much and it is only available in debug builds. So, I would suggest that we just remove it. But I prefer not to remove it as part of this change. If you want to I can send out a small pre-patch to remove the RotateCMSCollectionTypes feature. However, I think it might be a better workflow for me to keep the changes as they are in the patch and then follow up with removing RotateCMSCollectionTypes. Let me know what you prefer and I'll file a JBS bug to track it if we decide to remove it.
> 
> I do agree that the naming is bad. But if we decide to remove this functionality there is not much need to spend time on the naming discussion.

My inclination would be to remove it.  I think we have way too many options whose purpose is unclear and which are never being tested, so who knows whether they actually do what they were intended for (whatever that was), or do anything useful at all.  (Witness the option I recently removed that has not worked during it’s entire lifetime in the mercurial repository.)

> Updated webrev:
> http://cr.openjdk.java.net/~brutisso/8064702/webrev.02/
> 
> Diff compared to last webrev:
> http://cr.openjdk.java.net/~brutisso/8064702/webrev.01-02.diff/

New webrev looks good.