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

[Bengt Rutisson]

Hi Stefan,

Thanks for looking at this!


On 2014-11-12 14:43, Stefan Karlsson wrote:
> 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();
>    }
>  }

The _yield flag is a bit interesting. There are two closures for doing 
the marking, MarkFromRootsClosure and Par_MarkFromRootsClosure. The 
MarkFromRootsClosure yields based on the CMSYield flag as it is being 
used in CMSCollector::do_marking_st():

   MarkFromRootsClosure markFromRootsClosure(this, _span, &_markBitMap,
     &_markStack, CMSYield);

But the Par_MarkFromRootsClosure was setting the yield value based on 
the asynch value:

         // Do the marking work within a non-empty span --
         // the last argument to the constructor indicates whether the
         // iteration should be incremental with periodic yields.
         Par_MarkFromRootsClosure cl(this, _collector, my_span,
                                     &_collector->_markBitMap,
                                     work_queue(i),
                                     &_collector->_markStack,
                                     _asynch);

The _asynch value here came from CMSCollector::do_marking_mt() which 
after the removal of the foreground collector is always true. So, in the 
Par_MarkFromRootsClosure I could remove the _yield field since it is now 
always true. But in the MarkFromRootsClosure I need to leave it in to 
keep the same behavior as before with the CMSYield flag.

It seems a bit fishy to me that the two marking closure work 
differently, but I'd rather not change this behavior as part of this change.


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

Good catch. I removed these occurrences of asynch as well.

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/

Offline Stefan also suggested to rename the parameter to reset() from 
asynch to concurrent since this is the only usage left of asynch and 
concurrent seems like a more suitable name. This is also included in the 
webrev above.

Thanks,
Bengt


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