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

[Mikael Gerdin]

Hi Roman,

On 2017-07-05 13:58, Mikael Gerdin wrote:
> 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 just realized that your change doesn't build on Windows since you 
didn't #include "precompiled.hpp" in cmsHeap.cpp. MSVC is really picky 
about that.
/Mikael

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