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

[Stefan Karlsson]

Hi Bengt,

On 2014-11-12 14:03, Bengt Rutisson wrote:
>
> Hi everyone,
>
> Can I have a couple of reviews for this change to remove the 
> foreground collector in CMS?
>
> http://cr.openjdk.java.net/~brutisso/8064702/webrev.00/

I haven't completed the review, but here are some comments:

http://cr.openjdk.java.net/~brutisso/8064702/webrev.00/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.inline.hpp.udiff.html

Could you explain/motivate this change?:

  inline void Par_MarkFromRootsClosure::do_yield_check() {
    if (ConcurrentMarkSweepThread::should_yield() &&
-      !_collector->foregroundGCIsActive() &&
-      _yield) {
+      !_collector->foregroundGCIsActive()) {
      do_yield_work();
    }
  }


http://cr.openjdk.java.net/~brutisso/8064702/webrev.00/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp.frames.html

You removed the call to checkpointRootsInitialWork(bool asynch) that 
uses false as a parameter. I think you should remove that parameter 
(just like you did for the other functions) and remove the code that 
isn't executed anymore.

thanks,
StefanK

>
> https://bugs.openjdk.java.net/browse/JDK-8064702
>
> This work is part of JEP 214 (http://openjdk.java.net/jeps/214). There 
> are quite a few lines changed. Mostly it is just removal of code and 
> it should hopefully be fairly straight forward to review.
>
> A couple of things are worth noting:
>
> The name "foreground collection" is used for two different things in 
> CMS. Both the full GC that does a serial mark and compact of the whole 
> heap (the fall back to SerialGC) and the foreground mode of completing 
> a CMS cycle in a stop-the-world phase. We still need the fall back to 
> the SerialGC for when we get a concurrent mode failure, so this change 
> is just to remove the later feature. That is to remove the ability to 
> interrupt a concurrent CMS cycle and finish it up in a stopped manner.
>
> This means that there is still code and comments using the word 
> "foreground" even after the changes I propose. In particular the 
> protocol to synchronize between the CMS collector and the MarkSweep 
> serial full GC is left intact and uses locks and flags with foreground 
> names.
>
> The intent of the change is to remove all traces of the actual 
> foreground mode of the CMS cycle while changing as little as possible 
> of the SerialGC interaction.
>
> Since there were these two modes of the foreground collector before 
> many methods needed to know in which mode they were operating. This 
> state was communicated by passing around parameters that were often 
> called asynch and clear_all_soft_refs. Now that there is only one mode 
> (the SerialGC one) I've tried to remove these parameters as much as 
> possible. The interpretation being that asynch means the SerialGC (so 
> always true now) and that we should always clear all soft refs when we 
> do a full GC and never clear all soft refs when we do a concurrent CMS 
> cycle.
>
> Thanks,
> Bengt