RFR: 8179387: Factor out CMS specific code from GenCollectedHeap into its own subclass

[Mikael Gerdin]

Hi Roman,

On 2017-07-03 17:05, Roman Kennke wrote:
> Am 03.07.2017 um 11:13 schrieb Roman Kennke:
>> Am 03.07.2017 um 09:35 schrieb Mikael Gerdin:
>>> Hi Roman,
>>>
>>> On 2017-06-30 18:32, Roman Kennke wrote:
>>>> I came across one problem using this approach: We will have 2 instances
>>>> of CollectedHeap around, where there's usually only 1, and some code
>>>> expects only 1. For example, in CollectedHeap constructor, we create new
>>>> PerfData variables, and we now create them 2x, which leads to an assert
>>>> being thrown. I suspect there is more code like that.
>>>>
>>>> I will attempt to refactor this a little more, maybe it's not that bad,
>>>> but it's probably not worth spending too much time on it.
>>> I think refactoring the code to not expect a singleton CollectedHeap
>>> instance is a bit too much.
>>> Perhaps there is another way to share common code between Serial and
>>> CMS but that might require a bit more thought.
>> Yeah, definitely. I hit another difficulty: pretty much the same issues
>> that I'm having with GenCollectedHeap/CMSHeap/CollectedHeap now show up
>> with Generation and its subclasses..
>>
>> How about we push the original patch that I've posted, and work from
>> there? In fact, I *have* found some little things I would change (some
>> more if (UseConcMarkSweepGC) branches in GenCollectedHeap that I have
>> overlooked in my first pass...)
> 
> So here's the little change (two more places in genCollectedHeap.hpp
> where UseConcMarkSweepGC was used to alter behaviour:
> 
> http://cr.openjdk.java.net/~rkennke/8179387/webrev.02/
> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.02/>
> 
> Ok to push this?

I think this looks like a good step in the right direction!
One thing I noticed is that you can put "enum GCH_strong_roots_tasks" 
inside of GenCollectedHeap to avoid tainting the global namespace with 
the enum members. Just above the declaration of _process_strong_tasks 
seems like an excellent location for the enum declaration :)

This looks like it's not needed anymore.
bool CMSHeap::should_do_concurrent_full_gc(GCCause::Cause cause) {
   if (!UseConcMarkSweepGC) {
     return false;
   }

/Mikael

> 
> Roman
>