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

[Bengt Rutisson]

Hi Kim,

Thanks for looking at this!

On 2014-11-13 01:41, Kim Barrett wrote:
> On Nov 12, 2014, at 11:49 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
>> Here's an updated webrev:
>> http://cr.openjdk.java.net/~brutisso/8064702/webrev.01/
>>
>> And just the diff compared to the last webrev:
>> http://cr.openjdk.java.net/~brutisso/8064702/webrev.00-01.diff/
> ------------------------------------------------------------------------------
>
> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp
>
> 1150   // Does a "full" (forced) collection invoked on this generation collect
> 1151   // all younger generations as well? Note that the second conjunct is a
> 1152   // hack to allow the collection of the younger gen first if the flag is
> 1153   // set.
> 1154   virtual bool full_collects_younger_generations() const {
> 1155     return UseCMSCompactAtFullCollection && !ScavengeBeforeFullGC;
> 1156   }
>
> =>
>
> 1123   virtual bool full_collects_younger_generations() const {
> 1124     return !ScavengeBeforeFullGC;
> 1125   }
>
> The complete removal of the comment is eliminating the information in
> the final sentence, regarding the second conjunct (e.g. the code that
> is being retained) being a hack.  Presumably someone (long ago and
> lost in the mists of pre-mercurial source control) thought that test
> was a bit of a kludge and might deserve a rethink.
>
> Note that there is an identical comment on
> TenuredGeneration::full_collects_younger_generations(), and the updated
> implementation here is now identical to the implementation for
> TenuredGeneration.  Keeping the associated comments consistent for
> these identical functions would be nice.

Good point. I added the comment back. My motivation for removing the 
comment in the first place was just that I figured that the name of the 
method was pretty much saying the same thing as the comment. But you are 
right about the second conjunct and it seems better to just leave the 
comment in.

>
> ------------------------------------------------------------------------------
>
> 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.

>
> ------------------------------------------------------------------------------
>
> 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.

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/

Thanks again for looking at this!
Bengt

>
> ------------------------------------------------------------------------------
>