RFR: 8199925: Break out GC selection logic from GCArguments to GCSelector

[Erik Osterlund]

Hi Per,

Looks good.

Thanks,
/Erik

> On 26 Mar 2018, at 08:40, Per Liden <per.liden at oracle.com> wrote:
> 
> Hi again,
> 
> It turned out that the Solaris Studio compiler didn't like structs with reference members without a user-defined constructor (all other compilers happily swallows this), so I had to add one.
> 
> Updated webrev:
> 
> Diff: http://cr.openjdk.java.net/~pliden/8199925/webrev.1vs2
> Full: http://cr.openjdk.java.net/~pliden/8199925/webrev.2
> 
> /Per
> 
>> On 03/23/2018 11:57 AM, Per Liden wrote:
>> Thanks Roman and Erik for reviewing!
>> /Per
>>> On 03/21/2018 04:07 PM, Roman Kennke wrote:
>>> Looks good! Thank you!
>>> 
>>> Am 21. März 2018 14:11:52 MEZ schrieb Per Liden <per.liden at oracle.com>:
>>>> Hi,
>>>> 
>>>> Here's an updated webrev. I renamed GCSelector to GCConfig and it's now
>>>> 
>>>> also the goto place for getting hold of the GCArguments instance for
>>>> the
>>>> currently selected GC.
>>>> 
>>>> http://cr.openjdk.java.net/~pliden/8199925/webrev.1
>>>> 
>>>> Btw, this is built on top of JDK-8199850 (Move parsing of VerifyGCType
>>>> to G1) and JDK-8199918 (Shorten names of CollectedHeap::Name members),
>>>> and the GCConfig::is_gc_* functions will eventually be used by
>>>> JDK-8199927 (Make WhiteBox more GC agnostic).
>>>> 
>>>> /Per
>>>> 
>>>>> On 03/21/2018 11:33 AM, Per Liden wrote:
>>>>> Hi Roman,
>>>>> 
>>>>>> On 03/21/2018 10:15 AM, Roman Kennke wrote:
>>>>>>> Am 21.03.2018 um 09:12 schrieb Per Liden:
>>>>>>> In an attempt to make WhiteBox more GC agnostic, and in turn make
>>>> it
>>>>>>> easier to plugin new GC without touching a lot of non-GC code, this
>>>>>>> patch breaks out the GC selection logic from GCArguments to
>>>> GCSelector.
>>>>>>> There are two reasons why I think this makes sense:
>>>>>>> 1) The GC selection logic is self-contained and fairly unrelated to
>>>> the
>>>>>>> rest of the flags processing done by GCArguments and its
>>>> GC-specific
>>>>>>> children.
>>>>>>> 2) Parts of the GC selection logic is needed by WhiteBox to answer
>>>>>>> questions about which GC are supported, selected, etc.
>>>>>>> 
>>>>>>> A follow up patch (JDK-8199927) will change WhiteBox (WB_CurrentGC,
>>>>>>> WB_AllSupportedGC, WB_GCSelectedByErgo), to use the GCSelector.
>>>>>>> 
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8199925
>>>>>>> Webrev: http://cr.openjdk.java.net/~pliden/8199925/webrev.0
>>>>>>> 
>>>>>>> /Per
>>>>>> 
>>>>>> My original proposal for the GCArguments did have a separate
>>>>>> GCFactory. :-)
>>>>>> 
>>>>>> In generally I like your change.
>>>>>> 
>>>>>> I'd probably go even further and let GCSelector initialize and
>>>> return an
>>>>>> instance of GCArguments. This way we wouldn't end up with two places
>>>>>> that have knowledge of all possible GCs, but would only have one
>>>> place
>>>>>> (GCSelector) where one had to hook up for adding a new GC.
>>>>>> 
>>>>>> What do you think?
>>>>> 
>>>>> My intention here was that GCSelector would be a pure translator of
>>>>> Use*GC flags to CollectedHeap::Name, i.e. a small and well defined
>>>>> component, which knows nothing about GCArguments. The purpose of this
>>>> 
>>>>> was to clean out the WhiteBox stuff.
>>>>> 
>>>>> On the other hand, I see what you mean, and too would like to reach a
>>>> 
>>>>> state where we have one place in the code that "knows about all the
>>>> GCs".
>>>>> 
>>>>> Let me think about it some more and maybe prototype an alternative,
>>>> and
>>>>> we'll see if that gets us to a better place.
>>>>> 
>>>>> /Per
>>>>> 
>>>>>> 
>>>>>> Roman
>>>>>> 
>>>