Re: RFR: 8189171: Move GC argument processing into GC specific classes

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

David Holmes
Hi Roman,

Not a review as GC folk need to do that.

On 13/10/2017 5:59 AM, Roman Kennke wrote:
> I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev because it
> touches both areas (i.e. the GC interface).
>
> Currently, all GC related argument processing is done in arguments.cpp,
> littering it with #ifdef INCLUDE_ALL_GCS and all sorts of GC specific
> methods etc.

 From a runtime perspective I like all the GC specific ifdefs and
settings moved out-of-line of the main argument processing.

 From a refactoring perspective I noticed that set_object_alignment() no
longer calls set_cms_values(). I presume setting it elsewhere is okay?

Thanks,
David

> In order to have a cleaner GC interface, I propose to move all that code
> into GC specific classes under their GC specific directories.
>
> Since at this point in time we haven't created the XXXHeap subclasses
> yet (and cannot, before having set up all the flags and arguments), I
> propose to create a GC interface of which we create an instance as soon
> as we have selected a GC, and then do argument processing there.
>
> This 'GC' interface is, at this point, meant as a sort of GC factory
> interface. With this patch, it will only do argument processing. Later I
> plan to add heap creation to it (which is currently done in
> universe.cpp, with INCLUDE_ALL_GCS stuff and all the if (UseBlahGC) ..
> else if .. else branch chains. My current prototype in the jdk10 sandbox
> also provides servicabilty support classes and some more stuff, but we
> should decide later where to put this.
>
> I broke out some code paths into gc_factory.cpp (i.e. not in gc.cpp).
> Those are all the code paths that select GC, create the right instances,
> etc, in other words, the code paths that need to know about all GCs.
> Ideally, this should be the only file that needs to know about all the
> GCs. (Suggestions for improvement are welcome of course.)
>
> I tested this by running hotspot_gc jtreg tests, with normal fastdebug
> and no-precompiled configurations. Everything passes fine.
>
> http://cr.openjdk.java.net/~rkennke/8189171/webrev.00/ 
> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.00/>
>
> Opinions?
>
> Roman
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6

> I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev because
> it touches both areas (i.e. the GC interface).
>
> Currently, all GC related argument processing is done in
> arguments.cpp, littering it with #ifdef INCLUDE_ALL_GCS and all sorts
> of GC specific methods etc.
>
> In order to have a cleaner GC interface, I propose to move all that
> code into GC specific classes under their GC specific directories.
>
> Since at this point in time we haven't created the XXXHeap subclasses
> yet (and cannot, before having set up all the flags and arguments), I
> propose to create a GC interface of which we create an instance as
> soon as we have selected a GC, and then do argument processing there.
>
> This 'GC' interface is, at this point, meant as a sort of GC factory
> interface. With this patch, it will only do argument processing. Later
> I plan to add heap creation to it (which is currently done in
> universe.cpp, with INCLUDE_ALL_GCS stuff and all the if (UseBlahGC) ..
> else if .. else branch chains. My current prototype in the jdk10
> sandbox also provides servicabilty support classes and some more
> stuff, but we should decide later where to put this.
>
> I broke out some code paths into gc_factory.cpp (i.e. not in gc.cpp).
> Those are all the code paths that select GC, create the right
> instances, etc, in other words, the code paths that need to know about
> all GCs. Ideally, this should be the only file that needs to know
> about all the GCs. (Suggestions for improvement are welcome of course.)
>
> I tested this by running hotspot_gc jtreg tests, with normal fastdebug
> and no-precompiled configurations. Everything passes fine.
>
> http://cr.openjdk.java.net/~rkennke/8189171/webrev.00/ 
> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.00/>
>
> Opinions?
>
> Roman
>
Ping?

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Zhengyu Gu-2
In reply to this post by David Holmes
Hi Roman,

Look good overall.

1) Probably make sense to factor out static methods from GC into
separate GCFactory class.

2) Stylish:
   #ifndef SHARE_VM_GC_G1_G1_GC_HPP -> #ifndef SHARE_VM_GC_G1_G1GC_HPP

Thanks,

-Zhengyu

On 10/12/2017 03:59 PM, Roman Kennke wrote:

> I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev because it
> touches both areas (i.e. the GC interface).
>
> Currently, all GC related argument processing is done in arguments.cpp,
> littering it with #ifdef INCLUDE_ALL_GCS and all sorts of GC specific
> methods etc.
>
> In order to have a cleaner GC interface, I propose to move all that code
> into GC specific classes under their GC specific directories.
>
> Since at this point in time we haven't created the XXXHeap subclasses
> yet (and cannot, before having set up all the flags and arguments), I
> propose to create a GC interface of which we create an instance as soon
> as we have selected a GC, and then do argument processing there.
>
> This 'GC' interface is, at this point, meant as a sort of GC factory
> interface. With this patch, it will only do argument processing. Later I
> plan to add heap creation to it (which is currently done in
> universe.cpp, with INCLUDE_ALL_GCS stuff and all the if (UseBlahGC) ..
> else if .. else branch chains. My current prototype in the jdk10 sandbox
> also provides servicabilty support classes and some more stuff, but we
> should decide later where to put this.
>
> I broke out some code paths into gc_factory.cpp (i.e. not in gc.cpp).
> Those are all the code paths that select GC, create the right instances,
> etc, in other words, the code paths that need to know about all GCs.
> Ideally, this should be the only file that needs to know about all the
> GCs. (Suggestions for improvement are welcome of course.)
>
> I tested this by running hotspot_gc jtreg tests, with normal fastdebug
> and no-precompiled configurations. Everything passes fine.
>
> http://cr.openjdk.java.net/~rkennke/8189171/webrev.00/ 
> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.00/>
>
> Opinions?
>
> Roman
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6
Hi Zhengyu,

thanks for reviewing!

> Hi Roman,
>
> Look good overall.
>
> 1) Probably make sense to factor out static methods from GC into
> separate GCFactory class.
Right, this looks better.

> 2) Stylish:
>   #ifndef SHARE_VM_GC_G1_G1_GC_HPP -> #ifndef SHARE_VM_GC_G1_G1GC_HPP
Ok, right. I also removed the '_VM' parts to account for the changed
directory structure in mono-repo.

Differential:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.01.diff/ 
<http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01.diff/>
Full:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/ 
<http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/>

Better now?

Roman

>
> Thanks,
>
> -Zhengyu
>
> On 10/12/2017 03:59 PM, Roman Kennke wrote:
>> I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev because
>> it touches both areas (i.e. the GC interface).
>>
>> Currently, all GC related argument processing is done in
>> arguments.cpp, littering it with #ifdef INCLUDE_ALL_GCS and all sorts
>> of GC specific methods etc.
>>
>> In order to have a cleaner GC interface, I propose to move all that
>> code into GC specific classes under their GC specific directories.
>>
>> Since at this point in time we haven't created the XXXHeap subclasses
>> yet (and cannot, before having set up all the flags and arguments), I
>> propose to create a GC interface of which we create an instance as
>> soon as we have selected a GC, and then do argument processing there.
>>
>> This 'GC' interface is, at this point, meant as a sort of GC factory
>> interface. With this patch, it will only do argument processing.
>> Later I plan to add heap creation to it (which is currently done in
>> universe.cpp, with INCLUDE_ALL_GCS stuff and all the if (UseBlahGC)
>> .. else if .. else branch chains. My current prototype in the jdk10
>> sandbox also provides servicabilty support classes and some more
>> stuff, but we should decide later where to put this.
>>
>> I broke out some code paths into gc_factory.cpp (i.e. not in gc.cpp).
>> Those are all the code paths that select GC, create the right
>> instances, etc, in other words, the code paths that need to know
>> about all GCs. Ideally, this should be the only file that needs to
>> know about all the GCs. (Suggestions for improvement are welcome of
>> course.)
>>
>> I tested this by running hotspot_gc jtreg tests, with normal
>> fastdebug and no-precompiled configurations. Everything passes fine.
>>
>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.00/ 
>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.00/>
>>
>> Opinions?
>>
>> Roman
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Zhengyu Gu-2
Good to me.

-Zhengyu

On 10/17/2017 05:21 PM, Roman Kennke wrote:

> Hi Zhengyu,
>
> thanks for reviewing!
>
>> Hi Roman,
>>
>> Look good overall.
>>
>> 1) Probably make sense to factor out static methods from GC into
>> separate GCFactory class.
> Right, this looks better.
>
>> 2) Stylish:
>>   #ifndef SHARE_VM_GC_G1_G1_GC_HPP -> #ifndef SHARE_VM_GC_G1_G1GC_HPP
> Ok, right. I also removed the '_VM' parts to account for the changed
> directory structure in mono-repo.
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8189171/webrev.01.diff/ 
> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01.diff/>
> Full:
> http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/ 
> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/>
>
> Better now?
>
> Roman
>
>>
>> Thanks,
>>
>> -Zhengyu
>>
>> On 10/12/2017 03:59 PM, Roman Kennke wrote:
>>> I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev because
>>> it touches both areas (i.e. the GC interface).
>>>
>>> Currently, all GC related argument processing is done in
>>> arguments.cpp, littering it with #ifdef INCLUDE_ALL_GCS and all sorts
>>> of GC specific methods etc.
>>>
>>> In order to have a cleaner GC interface, I propose to move all that
>>> code into GC specific classes under their GC specific directories.
>>>
>>> Since at this point in time we haven't created the XXXHeap subclasses
>>> yet (and cannot, before having set up all the flags and arguments), I
>>> propose to create a GC interface of which we create an instance as
>>> soon as we have selected a GC, and then do argument processing there.
>>>
>>> This 'GC' interface is, at this point, meant as a sort of GC factory
>>> interface. With this patch, it will only do argument processing.
>>> Later I plan to add heap creation to it (which is currently done in
>>> universe.cpp, with INCLUDE_ALL_GCS stuff and all the if (UseBlahGC)
>>> .. else if .. else branch chains. My current prototype in the jdk10
>>> sandbox also provides servicabilty support classes and some more
>>> stuff, but we should decide later where to put this.
>>>
>>> I broke out some code paths into gc_factory.cpp (i.e. not in gc.cpp).
>>> Those are all the code paths that select GC, create the right
>>> instances, etc, in other words, the code paths that need to know
>>> about all GCs. Ideally, this should be the only file that needs to
>>> know about all the GCs. (Suggestions for improvement are welcome of
>>> course.)
>>>
>>> I tested this by running hotspot_gc jtreg tests, with normal
>>> fastdebug and no-precompiled configurations. Everything passes fine.
>>>
>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.00/ 
>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.00/>
>>>
>>> Opinions?
>>>
>>> Roman
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6
In reply to this post by David Holmes
Am 13.10.2017 um 03:20 schrieb David Holmes:

> Hi Roman,
>
> Not a review as GC folk need to do that.
>
> On 13/10/2017 5:59 AM, Roman Kennke wrote:
>> I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev because
>> it touches both areas (i.e. the GC interface).
>>
>> Currently, all GC related argument processing is done in
>> arguments.cpp, littering it with #ifdef INCLUDE_ALL_GCS and all sorts
>> of GC specific methods etc.
>
> From a runtime perspective I like all the GC specific ifdefs and
> settings moved out-of-line of the main argument processing.
>
> From a refactoring perspective I noticed that set_object_alignment()
> no longer calls set_cms_values(). I presume setting it elsewhere is okay?
I totally forgot to reply to this.

What's important is that the CMS alignment values are set after
set_object_alignment() figured out the object alignment. The patch moves
that a little further down the road to the beginning of its GC specific
argument processing, but from GC perspective should be the same. I
looked thoroughly through the involved code paths and cannot see what
could go wrong.

I need one more review. GC folks? The current webrev is:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/ 
<http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/>

Thanks,
Roman

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Erik Osterlund
Hi Roman,

I would like to keep the CollectedHeap as the facade interface to the
GC, like it is today, instead of having a new GC class making it two
facade interfaces.
Of course, the CollectedHeap may only be used as a facade after it has
been created. And now we are dealing with code before it was created
during the bootstrapping process.

So what I would like to have here is a class specifically for that,
rather than another GC facade interface. I think this is mostly an
exercise of finding a better name for the class you call "GC". Now I am
not an expert in coming up with good names myself, but some name that
indicates this is being used for flag processing would be good. Perhaps
GCArgumentProcessor. The benefit of calling it something like that is
that if a GC requires argument processing later in the bootstrapping,
possibly after CollectedHeap has been created, it can still be used for
this purpose without having to feel weird about it.

How would you feel about that? Does it seem to make sense?

Thanks,
/Erik

On 2017-10-19 21:14, Roman Kennke wrote:

> Am 13.10.2017 um 03:20 schrieb David Holmes:
>> Hi Roman,
>>
>> Not a review as GC folk need to do that.
>>
>> On 13/10/2017 5:59 AM, Roman Kennke wrote:
>>> I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev
>>> because it touches both areas (i.e. the GC interface).
>>>
>>> Currently, all GC related argument processing is done in
>>> arguments.cpp, littering it with #ifdef INCLUDE_ALL_GCS and all
>>> sorts of GC specific methods etc.
>>
>> From a runtime perspective I like all the GC specific ifdefs and
>> settings moved out-of-line of the main argument processing.
>>
>> From a refactoring perspective I noticed that set_object_alignment()
>> no longer calls set_cms_values(). I presume setting it elsewhere is
>> okay?
> I totally forgot to reply to this.
>
> What's important is that the CMS alignment values are set after
> set_object_alignment() figured out the object alignment. The patch
> moves that a little further down the road to the beginning of its GC
> specific argument processing, but from GC perspective should be the
> same. I looked thoroughly through the involved code paths and cannot
> see what could go wrong.
>
> I need one more review. GC folks? The current webrev is:
> http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/ 
> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/>
>
> Thanks,
> Roman
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6
Hi Erik,

Yes that is fine.

Keep in mind that I'm planning to put heap creation into that class too.

I was thinking maybe to simply reverse the current naming: name the
all-static 'factory factory' 'GC', and thus call GC::initialize() and
GC::factory() or such, and the main interface GCFactory or
CollectedHeapFactory. What do you think?

Roman


> I would like to keep the CollectedHeap as the facade interface to the
> GC, like it is today, instead of having a new GC class making it two
> facade interfaces.
> Of course, the CollectedHeap may only be used as a facade after it has
> been created. And now we are dealing with code before it was created
> during the bootstrapping process.
>
> So what I would like to have here is a class specifically for that,
> rather than another GC facade interface. I think this is mostly an
> exercise of finding a better name for the class you call "GC". Now I
> am not an expert in coming up with good names myself, but some name
> that indicates this is being used for flag processing would be good.
> Perhaps GCArgumentProcessor. The benefit of calling it something like
> that is that if a GC requires argument processing later in the
> bootstrapping, possibly after CollectedHeap has been created, it can
> still be used for this purpose without having to feel weird about it.
>
> How would you feel about that? Does it seem to make sense?
>
> Thanks,
> /Erik
>
> On 2017-10-19 21:14, Roman Kennke wrote:
>> Am 13.10.2017 um 03:20 schrieb David Holmes:
>>> Hi Roman,
>>>
>>> Not a review as GC folk need to do that.
>>>
>>> On 13/10/2017 5:59 AM, Roman Kennke wrote:
>>>> I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev
>>>> because it touches both areas (i.e. the GC interface).
>>>>
>>>> Currently, all GC related argument processing is done in
>>>> arguments.cpp, littering it with #ifdef INCLUDE_ALL_GCS and all
>>>> sorts of GC specific methods etc.
>>>
>>> From a runtime perspective I like all the GC specific ifdefs and
>>> settings moved out-of-line of the main argument processing.
>>>
>>> From a refactoring perspective I noticed that set_object_alignment()
>>> no longer calls set_cms_values(). I presume setting it elsewhere is
>>> okay?
>> I totally forgot to reply to this.
>>
>> What's important is that the CMS alignment values are set after
>> set_object_alignment() figured out the object alignment. The patch
>> moves that a little further down the road to the beginning of its GC
>> specific argument processing, but from GC perspective should be the
>> same. I looked thoroughly through the involved code paths and cannot
>> see what could go wrong.
>>
>> I need one more review. GC folks? The current webrev is:
>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/ 
>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/>
>>
>> Thanks,
>> Roman
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6
Hi Erik,

thanks for your suggestions. So I renamed 'GC' -> 'CollectedHeapFactory'
(and subclasses likewise) and 'GCFactory' to 'GCArgumentProcessor'. This
will make even more sense once I added heap creation to
CollectedHeapFactory (JDK-8189389).

Differential:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.02.diff/ 
<http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02.diff/>

Full webrev:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.02/ 
<http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02/>

Better?

Roman

> Hi Roman,
>
> I'm usually not one to really mind names too much, but I don't believe
> some bootstrapping code for the GC should hog the name "GC".
>
> Instead of GC::initialize(), I'm thinking a static
> GCArgumentProcessor::initialize() and instead of GC::factory(), I am
> thinking a static CollectedHeapFactory::create(). Or something like that.
>
> The reason is that I think the name GC is too general for the
> bootstraping-only problems its code touches. Hope I am making sense.
>
> Thanks,
> /Erik
>
> On 2017-10-20 14:25, Roman Kennke wrote:
>> Hi Erik,
>>
>> Yes that is fine.
>>
>> Keep in mind that I'm planning to put heap creation into that class too.
>>
>> I was thinking maybe to simply reverse the current naming: name the
>> all-static 'factory factory' 'GC', and thus call GC::initialize() and
>> GC::factory() or such, and the main interface GCFactory or
>> CollectedHeapFactory. What do you think?
>>
>> Roman
>>
>>
>>> I would like to keep the CollectedHeap as the facade interface to
>>> the GC, like it is today, instead of having a new GC class making it
>>> two facade interfaces.
>>> Of course, the CollectedHeap may only be used as a facade after it
>>> has been created. And now we are dealing with code before it was
>>> created during the bootstrapping process.
>>>
>>> So what I would like to have here is a class specifically for that,
>>> rather than another GC facade interface. I think this is mostly an
>>> exercise of finding a better name for the class you call "GC". Now I
>>> am not an expert in coming up with good names myself, but some name
>>> that indicates this is being used for flag processing would be good.
>>> Perhaps GCArgumentProcessor. The benefit of calling it something
>>> like that is that if a GC requires argument processing later in the
>>> bootstrapping, possibly after CollectedHeap has been created, it can
>>> still be used for this purpose without having to feel weird about it.
>>>
>>> How would you feel about that? Does it seem to make sense?
>>>
>>> Thanks,
>>> /Erik
>>>
>>> On 2017-10-19 21:14, Roman Kennke wrote:
>>>> Am 13.10.2017 um 03:20 schrieb David Holmes:
>>>>> Hi Roman,
>>>>>
>>>>> Not a review as GC folk need to do that.
>>>>>
>>>>> On 13/10/2017 5:59 AM, Roman Kennke wrote:
>>>>>> I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev
>>>>>> because it touches both areas (i.e. the GC interface).
>>>>>>
>>>>>> Currently, all GC related argument processing is done in
>>>>>> arguments.cpp, littering it with #ifdef INCLUDE_ALL_GCS and all
>>>>>> sorts of GC specific methods etc.
>>>>>
>>>>> From a runtime perspective I like all the GC specific ifdefs and
>>>>> settings moved out-of-line of the main argument processing.
>>>>>
>>>>> From a refactoring perspective I noticed that
>>>>> set_object_alignment() no longer calls set_cms_values(). I presume
>>>>> setting it elsewhere is okay?
>>>> I totally forgot to reply to this.
>>>>
>>>> What's important is that the CMS alignment values are set after
>>>> set_object_alignment() figured out the object alignment. The patch
>>>> moves that a little further down the road to the beginning of its
>>>> GC specific argument processing, but from GC perspective should be
>>>> the same. I looked thoroughly through the involved code paths and
>>>> cannot see what could go wrong.
>>>>
>>>> I need one more review. GC folks? The current webrev is:
>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/ 
>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/>
>>>>
>>>> Thanks,
>>>> Roman
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6
Hi Erik,

thank you for your suggestions and changes.

I like it: if you look at webrev.00 you will notice that my original proposal had both the static stuff and the virtual stuff in the same class too. So from there it's mostly the renaming exercise :-)

So let me just put your changes up for review (again), if you don't mind:

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/

Incremental over webrev.02:
http://cr.openjdk.java.net/~eosterlund/8189171/webrev.02_03/

So I suppose we can count it by reviewed ok by you?

Then it requires one more review to go in, right?

Thanks, Roman


Hi Roman,

Definitely looks better. So the static GCArgumentProcessor has a singleton GCCollectedHeapFactory that is created during GCArgumentProcessor::initialize().
That makes sense to me, but I think it can be even better.

First off - sorry for bikeshedding a bit here.

To save you from having to update the code due to my bikeshedding, I have prepared a patch to show you what I meant, and you can tell me whether you agree or not.

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/

Incremental over yours:
http://cr.openjdk.java.net/~eosterlund/8189171/webrev.02_03/

My changes are the following:

I collapsed the all-static GCArgumentProcessor and the CollectedHeapFactory into the same class called GCArguments (the natural GC helper for Arguments). It's a singleton object that deals with GC arguments stuff.

GCArguments has some static functions to create_instance(), creating the singleton GCArguments object, and after you have created the singleton instance, you access it through GCArguments::instance(), and it will point to a GC-specific GCArguments, e.g. G1Arguments, CMSArguments, etc.

The main reason why I think it's better to have a GC-specific GCArguments class compared to a CollectedHeapFactory class, is that I would like to eventually move all arguments processing into the GCArguments class, regardless of where (before or after CollectedHeap exists) in the boot strapping process. For example, today some argument stuff is dumped in the arguments.cpp file, and other stuff is dumped in CollectorPolicy::initialize_flags which is called later when the heap exists where it seemingly reads values out from flags, into internal variables, then changes them to what they should be and write the values back to the flags, and subsequently asserts that the internal and flag variants have the same value. Seems to me like we could in a subsequent refactoring move some of that kind of stuff into GCArguments instead, so that the argument setting logic is contained in the same class rather than split all over the place. But that seems like a separate RFE.

Other than that, noticed some precompile header include was missing, the GCArgumentProcessor::initialize() function returned jint but it always returned JNI_OK, and called fatal() if something went wrong, instead of logging an error and returning JNI_ERR. And now that these types are collapsed, I moved initialize_flags_global into the abstract initial_flags, allowing GCs to override it completely if necessary, but in practice having them now call the super initial_flags(). So yeah, went ahead and fixed that for you.

As for your subsequent patch creating the heap, that will just be a virtual call into the specific GCArguments class.

I hope you agree with my proposal. It seemed easier for me to provide a patch describing what I'm thinking than to write it down in text.

Thanks,
/Erik

On 2017-10-23 17:19, Roman Kennke wrote:
Hi Erik,

thanks for your suggestions. So I renamed 'GC' -> 'CollectedHeapFactory' (and subclasses likewise) and 'GCFactory' to 'GCArgumentProcessor'. This will make even more sense once I added heap creation to CollectedHeapFactory (JDK-8189389).

Differential:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.02.diff/ <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02.diff/>

Full webrev:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.02/ <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02/>

Better?

Roman

Hi Roman,

I'm usually not one to really mind names too much, but I don't believe some bootstrapping code for the GC should hog the name "GC".

Instead of GC::initialize(), I'm thinking a static GCArgumentProcessor::initialize() and instead of GC::factory(), I am thinking a static CollectedHeapFactory::create(). Or something like that.

The reason is that I think the name GC is too general for the bootstraping-only problems its code touches. Hope I am making sense.

Thanks,
/Erik

On 2017-10-20 14:25, Roman Kennke wrote:
Hi Erik,

Yes that is fine.

Keep in mind that I'm planning to put heap creation into that class too.

I was thinking maybe to simply reverse the current naming: name the all-static 'factory factory' 'GC', and thus call GC::initialize() and GC::factory() or such, and the main interface GCFactory or CollectedHeapFactory. What do you think?

Roman


I would like to keep the CollectedHeap as the facade interface to the GC, like it is today, instead of having a new GC class making it two facade interfaces.
Of course, the CollectedHeap may only be used as a facade after it has been created. And now we are dealing with code before it was created during the bootstrapping process.

So what I would like to have here is a class specifically for that, rather than another GC facade interface. I think this is mostly an exercise of finding a better name for the class you call "GC". Now I am not an expert in coming up with good names myself, but some name that indicates this is being used for flag processing would be good. Perhaps GCArgumentProcessor. The benefit of calling it something like that is that if a GC requires argument processing later in the bootstrapping, possibly after CollectedHeap has been created, it can still be used for this purpose without having to feel weird about it.

How would you feel about that? Does it seem to make sense?

Thanks,
/Erik

On 2017-10-19 21:14, Roman Kennke wrote:
Am 13.10.2017 um 03:20 schrieb David Holmes:
Hi Roman,

Not a review as GC folk need to do that.

On 13/10/2017 5:59 AM, Roman Kennke wrote:
I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev because it touches both areas (i.e. the GC interface).

Currently, all GC related argument processing is done in arguments.cpp, littering it with #ifdef INCLUDE_ALL_GCS and all sorts of GC specific methods etc.

From a runtime perspective I like all the GC specific ifdefs and settings moved out-of-line of the main argument processing.

From a refactoring perspective I noticed that set_object_alignment() no longer calls set_cms_values(). I presume setting it elsewhere is okay?
I totally forgot to reply to this.

What's important is that the CMS alignment values are set after set_object_alignment() figured out the object alignment. The patch moves that a little further down the road to the beginning of its GC specific argument processing, but from GC perspective should be the same. I looked thoroughly through the involved code paths and cannot see what could go wrong.

I need one more review. GC folks? The current webrev is:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/ <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/>

Thanks,
Roman







Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Erik Osterlund
Hi Roman,

On 25 Oct 2017, at 17:54, Roman Kennke <[hidden email]> wrote:

Hi Erik,

thank you for your suggestions and changes.

I like it: if you look at webrev.00 you will notice that my original proposal had both the static stuff and the virtual stuff in the same class too. So from there it's mostly the renaming exercise :-)

Yeah. Sounds like we are aligned.

So let me just put your changes up for review (again), if you don't mind:

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/

Incremental over webrev.02:
http://cr.openjdk.java.net/~eosterlund/8189171/webrev.02_03/

So I suppose we can count it by reviewed ok by you?

Yes. Looks fantastic.

Then it requires one more review to go in, right?

Yup.

Thanks,
/Erik

Thanks, Roman


Hi Roman,

Definitely looks better. So the static GCArgumentProcessor has a singleton GCCollectedHeapFactory that is created during GCArgumentProcessor::initialize().
That makes sense to me, but I think it can be even better.

First off - sorry for bikeshedding a bit here.

To save you from having to update the code due to my bikeshedding, I have prepared a patch to show you what I meant, and you can tell me whether you agree or not.

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/

Incremental over yours:
http://cr.openjdk.java.net/~eosterlund/8189171/webrev.02_03/

My changes are the following:

I collapsed the all-static GCArgumentProcessor and the CollectedHeapFactory into the same class called GCArguments (the natural GC helper for Arguments). It's a singleton object that deals with GC arguments stuff.

GCArguments has some static functions to create_instance(), creating the singleton GCArguments object, and after you have created the singleton instance, you access it through GCArguments::instance(), and it will point to a GC-specific GCArguments, e.g. G1Arguments, CMSArguments, etc.

The main reason why I think it's better to have a GC-specific GCArguments class compared to a CollectedHeapFactory class, is that I would like to eventually move all arguments processing into the GCArguments class, regardless of where (before or after CollectedHeap exists) in the boot strapping process. For example, today some argument stuff is dumped in the arguments.cpp file, and other stuff is dumped in CollectorPolicy::initialize_flags which is called later when the heap exists where it seemingly reads values out from flags, into internal variables, then changes them to what they should be and write the values back to the flags, and subsequently asserts that the internal and flag variants have the same value. Seems to me like we could in a subsequent refactoring move some of that kind of stuff into GCArguments instead, so that the argument setting logic is contained in the same class rather than split all over the place. But that seems like a separate RFE.

Other than that, noticed some precompile header include was missing, the GCArgumentProcessor::initialize() function returned jint but it always returned JNI_OK, and called fatal() if something went wrong, instead of logging an error and returning JNI_ERR. And now that these types are collapsed, I moved initialize_flags_global into the abstract initial_flags, allowing GCs to override it completely if necessary, but in practice having them now call the super initial_flags(). So yeah, went ahead and fixed that for you.

As for your subsequent patch creating the heap, that will just be a virtual call into the specific GCArguments class.

I hope you agree with my proposal. It seemed easier for me to provide a patch describing what I'm thinking than to write it down in text.

Thanks,
/Erik

On 2017-10-23 17:19, Roman Kennke wrote:
Hi Erik,

thanks for your suggestions. So I renamed 'GC' -> 'CollectedHeapFactory' (and subclasses likewise) and 'GCFactory' to 'GCArgumentProcessor'. This will make even more sense once I added heap creation to CollectedHeapFactory (JDK-8189389).

Differential:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.02.diff/ <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02.diff/>

Full webrev:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.02/ <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02/>

Better?

Roman

Hi Roman,

I'm usually not one to really mind names too much, but I don't believe some bootstrapping code for the GC should hog the name "GC".

Instead of GC::initialize(), I'm thinking a static GCArgumentProcessor::initialize() and instead of GC::factory(), I am thinking a static CollectedHeapFactory::create(). Or something like that.

The reason is that I think the name GC is too general for the bootstraping-only problems its code touches. Hope I am making sense.

Thanks,
/Erik

On 2017-10-20 14:25, Roman Kennke wrote:
Hi Erik,

Yes that is fine.

Keep in mind that I'm planning to put heap creation into that class too.

I was thinking maybe to simply reverse the current naming: name the all-static 'factory factory' 'GC', and thus call GC::initialize() and GC::factory() or such, and the main interface GCFactory or CollectedHeapFactory. What do you think?

Roman


I would like to keep the CollectedHeap as the facade interface to the GC, like it is today, instead of having a new GC class making it two facade interfaces.
Of course, the CollectedHeap may only be used as a facade after it has been created. And now we are dealing with code before it was created during the bootstrapping process.

So what I would like to have here is a class specifically for that, rather than another GC facade interface. I think this is mostly an exercise of finding a better name for the class you call "GC". Now I am not an expert in coming up with good names myself, but some name that indicates this is being used for flag processing would be good. Perhaps GCArgumentProcessor. The benefit of calling it something like that is that if a GC requires argument processing later in the bootstrapping, possibly after CollectedHeap has been created, it can still be used for this purpose without having to feel weird about it.

How would you feel about that? Does it seem to make sense?

Thanks,
/Erik

On 2017-10-19 21:14, Roman Kennke wrote:
Am 13.10.2017 um 03:20 schrieb David Holmes:
Hi Roman,

Not a review as GC folk need to do that.

On 13/10/2017 5:59 AM, Roman Kennke wrote:
I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev because it touches both areas (i.e. the GC interface).

Currently, all GC related argument processing is done in arguments.cpp, littering it with #ifdef INCLUDE_ALL_GCS and all sorts of GC specific methods etc.

From a runtime perspective I like all the GC specific ifdefs and settings moved out-of-line of the main argument processing.

From a refactoring perspective I noticed that set_object_alignment() no longer calls set_cms_values(). I presume setting it elsewhere is okay?
I totally forgot to reply to this.

What's important is that the CMS alignment values are set after set_object_alignment() figured out the object alignment. The patch moves that a little further down the road to the beginning of its GC specific argument processing, but from GC perspective should be the same. I looked thoroughly through the involved code paths and cannot see what could go wrong.

I need one more review. GC folks? The current webrev is:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/ <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/>

Thanks,
Roman







Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6
Am 26.10.2017 um 11:43 schrieb Per Liden:

> Hi,
>
> On 2017-10-25 18:11, Erik Osterlund wrote:
> [...]
>>> So let me just put your changes up for review (again), if you don't
>>> mind:
>>>
>>> Full webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
>>> <http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/>
>
> I like this. Just two naming suggestions:
>
> src/hotspot/share/gc/shared/gcArguments.hpp:
>
>   39   static jint create_instance();
>   40   static bool is_initialized();
>   41   static GCArguments* instance();
>
> Could we call these:
>
>   39   static jint initialize();
>   40   static bool is_initialized();
>   41   static GCArguments* arguments();
>
> Reasoning: initialize() maps better to its companion is_initialized().
> GCArguments::arguments() maps better to the existing patterns we have
> with CollectedHeap::heap().
Ok, that sounds good to me. Actually, it's almost full-circle back to my
original proposal ;-)

Differential:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.04.diff/ 
<http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04.diff/>
Full:
http://cr.openjdk.java.net/~rkennke/8189171/webrev.04/ 
<http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04/>

Ok to push now?

Roman
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Erik Osterlund
Hi Roman,

Looks good.

Thanks,
/Erik

On 2017-11-06 12:13, Roman Kennke wrote:

> Am 26.10.2017 um 11:43 schrieb Per Liden:
>> Hi,
>>
>> On 2017-10-25 18:11, Erik Osterlund wrote:
>> [...]
>>>> So let me just put your changes up for review (again), if you don't
>>>> mind:
>>>>
>>>> Full webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
>>>> <http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/>
>>
>> I like this. Just two naming suggestions:
>>
>> src/hotspot/share/gc/shared/gcArguments.hpp:
>>
>>   39   static jint create_instance();
>>   40   static bool is_initialized();
>>   41   static GCArguments* instance();
>>
>> Could we call these:
>>
>>   39   static jint initialize();
>>   40   static bool is_initialized();
>>   41   static GCArguments* arguments();
>>
>> Reasoning: initialize() maps better to its companion
>> is_initialized(). GCArguments::arguments() maps better to the
>> existing patterns we have with CollectedHeap::heap().
> Ok, that sounds good to me. Actually, it's almost full-circle back to
> my original proposal ;-)
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04.diff/ 
> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04.diff/>
> Full:
> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04/ 
> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04/>
>
> Ok to push now?
>
> Roman

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Per Liden
In reply to this post by Roman Kennke-6
Looks good Roman!

/Per

On 2017-11-06 12:13, Roman Kennke wrote:

> Am 26.10.2017 um 11:43 schrieb Per Liden:
>> Hi,
>>
>> On 2017-10-25 18:11, Erik Osterlund wrote:
>> [...]
>>>> So let me just put your changes up for review (again), if you don't
>>>> mind:
>>>>
>>>> Full webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
>>>> <http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/>
>>
>> I like this. Just two naming suggestions:
>>
>> src/hotspot/share/gc/shared/gcArguments.hpp:
>>
>>   39   static jint create_instance();
>>   40   static bool is_initialized();
>>   41   static GCArguments* instance();
>>
>> Could we call these:
>>
>>   39   static jint initialize();
>>   40   static bool is_initialized();
>>   41   static GCArguments* arguments();
>>
>> Reasoning: initialize() maps better to its companion is_initialized().
>> GCArguments::arguments() maps better to the existing patterns we have
>> with CollectedHeap::heap().
> Ok, that sounds good to me. Actually, it's almost full-circle back to my
> original proposal ;-)
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04.diff/
> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04.diff/>
> Full:
> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04/
> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04/>
>
> Ok to push now?
>
> Roman
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6
Hi Per & Erik,

thanks for the reviews!

Now I need a sponsor.

Final webrev (same as before, including Reviewed-by line):
http://cr.openjdk.java.net/~rkennke/8189171/webrev.05/ 
<http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.05/>

Thanks, Roman


> Looks good Roman!
>
> /Per
>
> On 2017-11-06 12:13, Roman Kennke wrote:
>> Am 26.10.2017 um 11:43 schrieb Per Liden:
>>> Hi,
>>>
>>> On 2017-10-25 18:11, Erik Osterlund wrote:
>>> [...]
>>>>> So let me just put your changes up for review (again), if you don't
>>>>> mind:
>>>>>
>>>>> Full webrev:
>>>>> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
>>>>> <http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/>
>>>
>>> I like this. Just two naming suggestions:
>>>
>>> src/hotspot/share/gc/shared/gcArguments.hpp:
>>>
>>>   39   static jint create_instance();
>>>   40   static bool is_initialized();
>>>   41   static GCArguments* instance();
>>>
>>> Could we call these:
>>>
>>>   39   static jint initialize();
>>>   40   static bool is_initialized();
>>>   41   static GCArguments* arguments();
>>>
>>> Reasoning: initialize() maps better to its companion is_initialized().
>>> GCArguments::arguments() maps better to the existing patterns we have
>>> with CollectedHeap::heap().
>> Ok, that sounds good to me. Actually, it's almost full-circle back to my
>> original proposal ;-)
>>
>> Differential:
>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04.diff/
>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04.diff/>
>> Full:
>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04/
>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04/>
>>
>> Ok to push now?
>>
>> Roman


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6
Hi Bob,
thanks for letting me know.

I filed: https://bugs.openjdk.java.net/browse/JDK-8191424
and posted for review:
http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-November/020784.html

Thanks, Roman

> Roman,
>
> It looks like this change may have caused the build of the minimal VM to
> fail.  The minimal VM is no longer a configuration that we regularly build and test
> but it is still a build option that can be selected via "--with-jvm-variants=minimal1"
>
>
> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp: In static member function 'static jint GCArguments::initialize()':
> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:113:17: error: 'defaultStream' has not been declared
>       jio_fprintf(defaultStream::error_stream(), "UseParallelGC not supported in this VM.\n");
>                   ^
> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:116:17: error: 'defaultStream' has not been declared
>       jio_fprintf(defaultStream::error_stream(), "UseG1GC not supported in this VM.\n");
>                   ^
> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:119:17: error: 'defaultStream' has not been declared
>       jio_fprintf(defaultStream::error_stream(), "UseConcMarkSweepGC not supported in this VM.\n");
>                   ^
> gmake[3]: *** [/scratch/users/bobv/jdk10hs/build/b00/linux-x64-minimal1/hotspot/variant-minimal/libjvm/objs/gcArguments.o] Error 1
>
> Bob.
>
>
>> On Nov 7, 2017, at 6:01 AM, Roman Kennke <[hidden email]> wrote:
>>
>> Hi Per & Erik,
>>
>> thanks for the reviews!
>>
>> Now I need a sponsor.
>>
>> Final webrev (same as before, including Reviewed-by line):
>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.05/ <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.05/>
>>
>> Thanks, Roman
>>
>>
>>> Looks good Roman!
>>>
>>> /Per
>>>
>>> On 2017-11-06 12:13, Roman Kennke wrote:
>>>> Am 26.10.2017 um 11:43 schrieb Per Liden:
>>>>> Hi,
>>>>>
>>>>> On 2017-10-25 18:11, Erik Osterlund wrote:
>>>>> [...]
>>>>>>> So let me just put your changes up for review (again), if you don't
>>>>>>> mind:
>>>>>>>
>>>>>>> Full webrev:
>>>>>>> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
>>>>>>> <http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/>
>>>>> I like this. Just two naming suggestions:
>>>>>
>>>>> src/hotspot/share/gc/shared/gcArguments.hpp:
>>>>>
>>>>>    39   static jint create_instance();
>>>>>    40   static bool is_initialized();
>>>>>    41   static GCArguments* instance();
>>>>>
>>>>> Could we call these:
>>>>>
>>>>>    39   static jint initialize();
>>>>>    40   static bool is_initialized();
>>>>>    41   static GCArguments* arguments();
>>>>>
>>>>> Reasoning: initialize() maps better to its companion is_initialized().
>>>>> GCArguments::arguments() maps better to the existing patterns we have
>>>>> with CollectedHeap::heap().
>>>> Ok, that sounds good to me. Actually, it's almost full-circle back to my
>>>> original proposal ;-)
>>>>
>>>> Differential:
>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04.diff/
>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04.diff/>
>>>> Full:
>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04/
>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04/>
>>>>
>>>> Ok to push now?
>>>>
>>>> Roman
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

David Holmes
On 17/11/2017 2:33 AM, Bob Vandette wrote:
> Yes, that’s the fix.  I built it correctly with that added include.

How? I can't build the MinimalVM in JPRT because the UNSUPPORTED_OPTION
macro is only locally defined in arguments.cpp, but is now used in
gcArguments.cpp:

/opt/jprt/T/P1/003543.daholme/s/open/src/hotspot/share/gc/shared/gcArguments.cpp:77:29:
error: 'UNSUPPORTED_OPTION' was not declared in this scope
    UNSUPPORTED_OPTION(UseG1GC);
                              ^
make[3]: ***
[/opt/jprt/T/P1/003543.daholme/s/build/linux-x86-debug/hotspot/variant-minimal/libjvm/objs/gcArguments.o]
Error 1

---

Roman/Erik: If changing code that has #include guards you must ensure
all the paths are tested!

Thanks,
David

> There are still a few other issues with building and using the minimal VM but those
> existed before your change.
>
> 1. You cannot build the minimal VM without also building the server VM.  There
> is a Makefile dependency on the server VM.
> 2. The jvm.cfg produced in the jvm and jre images does not add the minimalvm
> as one of the possible VM to select.  As a work-around jlink produces a workable jvm.cfg.
>
> Thanks,
> Bob.
>
>
>> On Nov 16, 2017, at 11:17 AM, Roman Kennke <[hidden email]> wrote:
>>
>> Hi Bob,
>> thanks for letting me know.
>>
>> I filed: https://bugs.openjdk.java.net/browse/JDK-8191424
>> and posted for review: http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-November/020784.html
>>
>> Thanks, Roman
>>
>>> Roman,
>>>
>>> It looks like this change may have caused the build of the minimal VM to
>>> fail.  The minimal VM is no longer a configuration that we regularly build and test
>>> but it is still a build option that can be selected via "--with-jvm-variants=minimal1"
>>>
>>>
>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp: In static member function 'static jint GCArguments::initialize()':
>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:113:17: error: 'defaultStream' has not been declared
>>>       jio_fprintf(defaultStream::error_stream(), "UseParallelGC not supported in this VM.\n");
>>>                   ^
>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:116:17: error: 'defaultStream' has not been declared
>>>       jio_fprintf(defaultStream::error_stream(), "UseG1GC not supported in this VM.\n");
>>>                   ^
>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:119:17: error: 'defaultStream' has not been declared
>>>       jio_fprintf(defaultStream::error_stream(), "UseConcMarkSweepGC not supported in this VM.\n");
>>>                   ^
>>> gmake[3]: *** [/scratch/users/bobv/jdk10hs/build/b00/linux-x64-minimal1/hotspot/variant-minimal/libjvm/objs/gcArguments.o] Error 1
>>>
>>> Bob.
>>>
>>>
>>>> On Nov 7, 2017, at 6:01 AM, Roman Kennke <[hidden email]> wrote:
>>>>
>>>> Hi Per & Erik,
>>>>
>>>> thanks for the reviews!
>>>>
>>>> Now I need a sponsor.
>>>>
>>>> Final webrev (same as before, including Reviewed-by line):
>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.05/ <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.05/>
>>>>
>>>> Thanks, Roman
>>>>
>>>>
>>>>> Looks good Roman!
>>>>>
>>>>> /Per
>>>>>
>>>>> On 2017-11-06 12:13, Roman Kennke wrote:
>>>>>> Am 26.10.2017 um 11:43 schrieb Per Liden:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 2017-10-25 18:11, Erik Osterlund wrote:
>>>>>>> [...]
>>>>>>>>> So let me just put your changes up for review (again), if you don't
>>>>>>>>> mind:
>>>>>>>>>
>>>>>>>>> Full webrev:
>>>>>>>>> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
>>>>>>>>> <http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/>
>>>>>>> I like this. Just two naming suggestions:
>>>>>>>
>>>>>>> src/hotspot/share/gc/shared/gcArguments.hpp:
>>>>>>>
>>>>>>>    39   static jint create_instance();
>>>>>>>    40   static bool is_initialized();
>>>>>>>    41   static GCArguments* instance();
>>>>>>>
>>>>>>> Could we call these:
>>>>>>>
>>>>>>>    39   static jint initialize();
>>>>>>>    40   static bool is_initialized();
>>>>>>>    41   static GCArguments* arguments();
>>>>>>>
>>>>>>> Reasoning: initialize() maps better to its companion is_initialized().
>>>>>>> GCArguments::arguments() maps better to the existing patterns we have
>>>>>>> with CollectedHeap::heap().
>>>>>> Ok, that sounds good to me. Actually, it's almost full-circle back to my
>>>>>> original proposal ;-)
>>>>>>
>>>>>> Differential:
>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04.diff/
>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04.diff/>
>>>>>> Full:
>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04/
>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04/>
>>>>>>
>>>>>> Ok to push now?
>>>>>>
>>>>>> Roman
>>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

David Holmes
Typo ...

On 20/11/2017 11:09 AM, David Holmes wrote:

> On 17/11/2017 2:33 AM, Bob Vandette wrote:
>> Yes, that’s the fix.  I built it correctly with that added include.
>
> How? I can't build the MinimalVM in JPRT because the UNSUPPORTED_OPTION
> macro is only locally defined in arguments.cpp, but is now used in
> gcArguments.cpp:
>
> /opt/jprt/T/P1/003543.daholme/s/open/src/hotspot/share/gc/shared/gcArguments.cpp:77:29:
> error: 'UNSUPPORTED_OPTION' was not declared in this scope
>     UNSUPPORTED_OPTION(UseG1GC);
>                               ^
> make[3]: ***
> [/opt/jprt/T/P1/003543.daholme/s/build/linux-x86-debug/hotspot/variant-minimal/libjvm/objs/gcArguments.o]
> Error 1
>
> ---
>
> Roman/Erik: If changing code that has #include guards you must ensure
> all the paths are tested!

#include -> #ifdef

David

> Thanks,
> David
>
>> There are still a few other issues with building and using the minimal
>> VM but those
>> existed before your change.
>>
>> 1. You cannot build the minimal VM without also building the server
>> VM.  There
>> is a Makefile dependency on the server VM.
>> 2. The jvm.cfg produced in the jvm and jre images does not add the
>> minimalvm
>> as one of the possible VM to select.  As a work-around jlink produces
>> a workable jvm.cfg.
>>
>> Thanks,
>> Bob.
>>
>>
>>> On Nov 16, 2017, at 11:17 AM, Roman Kennke <[hidden email]> wrote:
>>>
>>> Hi Bob,
>>> thanks for letting me know.
>>>
>>> I filed: https://bugs.openjdk.java.net/browse/JDK-8191424
>>> and posted for review:
>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-November/020784.html 
>>>
>>>
>>> Thanks, Roman
>>>
>>>> Roman,
>>>>
>>>> It looks like this change may have caused the build of the minimal
>>>> VM to
>>>> fail.  The minimal VM is no longer a configuration that we regularly
>>>> build and test
>>>> but it is still a build option that can be selected via
>>>> "--with-jvm-variants=minimal1"
>>>>
>>>>
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:
>>>> In static member function 'static jint GCArguments::initialize()':
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:113:17:
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(), "UseParallelGC not
>>>> supported in this VM.\n");
>>>>                   ^
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:116:17:
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(), "UseG1GC not
>>>> supported in this VM.\n");
>>>>                   ^
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:119:17:
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(), "UseConcMarkSweepGC
>>>> not supported in this VM.\n");
>>>>                   ^
>>>> gmake[3]: ***
>>>> [/scratch/users/bobv/jdk10hs/build/b00/linux-x64-minimal1/hotspot/variant-minimal/libjvm/objs/gcArguments.o]
>>>> Error 1
>>>>
>>>> Bob.
>>>>
>>>>
>>>>> On Nov 7, 2017, at 6:01 AM, Roman Kennke <[hidden email]> wrote:
>>>>>
>>>>> Hi Per & Erik,
>>>>>
>>>>> thanks for the reviews!
>>>>>
>>>>> Now I need a sponsor.
>>>>>
>>>>> Final webrev (same as before, including Reviewed-by line):
>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.05/ 
>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.05/>
>>>>>
>>>>> Thanks, Roman
>>>>>
>>>>>
>>>>>> Looks good Roman!
>>>>>>
>>>>>> /Per
>>>>>>
>>>>>> On 2017-11-06 12:13, Roman Kennke wrote:
>>>>>>> Am 26.10.2017 um 11:43 schrieb Per Liden:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 2017-10-25 18:11, Erik Osterlund wrote:
>>>>>>>> [...]
>>>>>>>>>> So let me just put your changes up for review (again), if you
>>>>>>>>>> don't
>>>>>>>>>> mind:
>>>>>>>>>>
>>>>>>>>>> Full webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/>
>>>>>>>> I like this. Just two naming suggestions:
>>>>>>>>
>>>>>>>> src/hotspot/share/gc/shared/gcArguments.hpp:
>>>>>>>>
>>>>>>>>    39   static jint create_instance();
>>>>>>>>    40   static bool is_initialized();
>>>>>>>>    41   static GCArguments* instance();
>>>>>>>>
>>>>>>>> Could we call these:
>>>>>>>>
>>>>>>>>    39   static jint initialize();
>>>>>>>>    40   static bool is_initialized();
>>>>>>>>    41   static GCArguments* arguments();
>>>>>>>>
>>>>>>>> Reasoning: initialize() maps better to its companion
>>>>>>>> is_initialized().
>>>>>>>> GCArguments::arguments() maps better to the existing patterns we
>>>>>>>> have
>>>>>>>> with CollectedHeap::heap().
>>>>>>> Ok, that sounds good to me. Actually, it's almost full-circle
>>>>>>> back to my
>>>>>>> original proposal ;-)
>>>>>>>
>>>>>>> Differential:
>>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04.diff/
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04.diff/>
>>>>>>> Full:
>>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04/
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04/>
>>>>>>>
>>>>>>> Ok to push now?
>>>>>>>
>>>>>>> Roman
>>>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6
In reply to this post by David Holmes
Hi David,

I just built linux-x86_64-normal-minimal-slowdebug and it does not fail.
That's what I did when I tested the patch(es). It's not reasonable to
expect devs to try and build all possible combinations of jvm variants,
debug-levels, or even arches etc.

I'll file and fix a follow-up bug for it.

Roman

> On 17/11/2017 2:33 AM, Bob Vandette wrote:
>> Yes, that’s the fix.  I built it correctly with that added include.
>
> How? I can't build the MinimalVM in JPRT because the
> UNSUPPORTED_OPTION macro is only locally defined in arguments.cpp, but
> is now used in gcArguments.cpp:
>
> /opt/jprt/T/P1/003543.daholme/s/open/src/hotspot/share/gc/shared/gcArguments.cpp:77:29:
> error: 'UNSUPPORTED_OPTION' was not declared in this scope
>    UNSUPPORTED_OPTION(UseG1GC);
>                              ^
> make[3]: ***
> [/opt/jprt/T/P1/003543.daholme/s/build/linux-x86-debug/hotspot/variant-minimal/libjvm/objs/gcArguments.o]
> Error 1
>
> ---
>
> Roman/Erik: If changing code that has #include guards you must ensure
> all the paths are tested!
>
> Thanks,
> David
>
>> There are still a few other issues with building and using the
>> minimal VM but those
>> existed before your change.
>>
>> 1. You cannot build the minimal VM without also building the server
>> VM.  There
>> is a Makefile dependency on the server VM.
>> 2. The jvm.cfg produced in the jvm and jre images does not add the
>> minimalvm
>> as one of the possible VM to select.  As a work-around jlink produces
>> a workable jvm.cfg.
>>
>> Thanks,
>> Bob.
>>
>>
>>> On Nov 16, 2017, at 11:17 AM, Roman Kennke <[hidden email]> wrote:
>>>
>>> Hi Bob,
>>> thanks for letting me know.
>>>
>>> I filed: https://bugs.openjdk.java.net/browse/JDK-8191424
>>> and posted for review:
>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-November/020784.html
>>>
>>> Thanks, Roman
>>>
>>>> Roman,
>>>>
>>>> It looks like this change may have caused the build of the minimal
>>>> VM to
>>>> fail.  The minimal VM is no longer a configuration that we
>>>> regularly build and test
>>>> but it is still a build option that can be selected via
>>>> "--with-jvm-variants=minimal1"
>>>>
>>>>
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:
>>>> In static member function 'static jint GCArguments::initialize()':
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:113:17:
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(), "UseParallelGC not
>>>> supported in this VM.\n");
>>>>                   ^
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:116:17:
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(), "UseG1GC not
>>>> supported in this VM.\n");
>>>>                   ^
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:119:17:
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(),
>>>> "UseConcMarkSweepGC not supported in this VM.\n");
>>>>                   ^
>>>> gmake[3]: ***
>>>> [/scratch/users/bobv/jdk10hs/build/b00/linux-x64-minimal1/hotspot/variant-minimal/libjvm/objs/gcArguments.o]
>>>> Error 1
>>>>
>>>> Bob.
>>>>
>>>>
>>>>> On Nov 7, 2017, at 6:01 AM, Roman Kennke <[hidden email]> wrote:
>>>>>
>>>>> Hi Per & Erik,
>>>>>
>>>>> thanks for the reviews!
>>>>>
>>>>> Now I need a sponsor.
>>>>>
>>>>> Final webrev (same as before, including Reviewed-by line):
>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.05/ 
>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.05/>
>>>>>
>>>>> Thanks, Roman
>>>>>
>>>>>
>>>>>> Looks good Roman!
>>>>>>
>>>>>> /Per
>>>>>>
>>>>>> On 2017-11-06 12:13, Roman Kennke wrote:
>>>>>>> Am 26.10.2017 um 11:43 schrieb Per Liden:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 2017-10-25 18:11, Erik Osterlund wrote:
>>>>>>>> [...]
>>>>>>>>>> So let me just put your changes up for review (again), if you
>>>>>>>>>> don't
>>>>>>>>>> mind:
>>>>>>>>>>
>>>>>>>>>> Full webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/>
>>>>>>>> I like this. Just two naming suggestions:
>>>>>>>>
>>>>>>>> src/hotspot/share/gc/shared/gcArguments.hpp:
>>>>>>>>
>>>>>>>>    39   static jint create_instance();
>>>>>>>>    40   static bool is_initialized();
>>>>>>>>    41   static GCArguments* instance();
>>>>>>>>
>>>>>>>> Could we call these:
>>>>>>>>
>>>>>>>>    39   static jint initialize();
>>>>>>>>    40   static bool is_initialized();
>>>>>>>>    41   static GCArguments* arguments();
>>>>>>>>
>>>>>>>> Reasoning: initialize() maps better to its companion
>>>>>>>> is_initialized().
>>>>>>>> GCArguments::arguments() maps better to the existing patterns
>>>>>>>> we have
>>>>>>>> with CollectedHeap::heap().
>>>>>>> Ok, that sounds good to me. Actually, it's almost full-circle
>>>>>>> back to my
>>>>>>> original proposal ;-)
>>>>>>>
>>>>>>> Differential:
>>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04.diff/
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04.diff/>
>>>>>>> Full:
>>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04/
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04/>
>>>>>>>
>>>>>>> Ok to push now?
>>>>>>>
>>>>>>> Roman
>>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189171: Move GC argument processing into GC specific classes

Roman Kennke-6
In reply to this post by David Holmes
... and minimal-fastdebug fails with something else:

/home/rkennke/src/openjdk/jdk-hs/src/hotspot/cpu/x86/c1_Runtime1_x86.cpp:136:10:
error: 'call_offset' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
    return call_offset;

I guess I can fix this blindly by including arguments.hpp in
gcArguments.cpp and hope for the best?

Roman

> On 17/11/2017 2:33 AM, Bob Vandette wrote:
>> Yes, that’s the fix.  I built it correctly with that added include.
>
> How? I can't build the MinimalVM in JPRT because the
> UNSUPPORTED_OPTION macro is only locally defined in arguments.cpp, but
> is now used in gcArguments.cpp:
>
> /opt/jprt/T/P1/003543.daholme/s/open/src/hotspot/share/gc/shared/gcArguments.cpp:77:29:
> error: 'UNSUPPORTED_OPTION' was not declared in this scope
>    UNSUPPORTED_OPTION(UseG1GC);
>                              ^
> make[3]: ***
> [/opt/jprt/T/P1/003543.daholme/s/build/linux-x86-debug/hotspot/variant-minimal/libjvm/objs/gcArguments.o]
> Error 1
>
> ---
>
> Roman/Erik: If changing code that has #include guards you must ensure
> all the paths are tested!
>
> Thanks,
> David
>
>> There are still a few other issues with building and using the
>> minimal VM but those
>> existed before your change.
>>
>> 1. You cannot build the minimal VM without also building the server
>> VM.  There
>> is a Makefile dependency on the server VM.
>> 2. The jvm.cfg produced in the jvm and jre images does not add the
>> minimalvm
>> as one of the possible VM to select.  As a work-around jlink produces
>> a workable jvm.cfg.
>>
>> Thanks,
>> Bob.
>>
>>
>>> On Nov 16, 2017, at 11:17 AM, Roman Kennke <[hidden email]> wrote:
>>>
>>> Hi Bob,
>>> thanks for letting me know.
>>>
>>> I filed: https://bugs.openjdk.java.net/browse/JDK-8191424
>>> and posted for review:
>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-November/020784.html
>>>
>>> Thanks, Roman
>>>
>>>> Roman,
>>>>
>>>> It looks like this change may have caused the build of the minimal
>>>> VM to
>>>> fail.  The minimal VM is no longer a configuration that we
>>>> regularly build and test
>>>> but it is still a build option that can be selected via
>>>> "--with-jvm-variants=minimal1"
>>>>
>>>>
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:
>>>> In static member function 'static jint GCArguments::initialize()':
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:113:17:
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(), "UseParallelGC not
>>>> supported in this VM.\n");
>>>>                   ^
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:116:17:
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(), "UseG1GC not
>>>> supported in this VM.\n");
>>>>                   ^
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:119:17:
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(),
>>>> "UseConcMarkSweepGC not supported in this VM.\n");
>>>>                   ^
>>>> gmake[3]: ***
>>>> [/scratch/users/bobv/jdk10hs/build/b00/linux-x64-minimal1/hotspot/variant-minimal/libjvm/objs/gcArguments.o]
>>>> Error 1
>>>>
>>>> Bob.
>>>>
>>>>
>>>>> On Nov 7, 2017, at 6:01 AM, Roman Kennke <[hidden email]> wrote:
>>>>>
>>>>> Hi Per & Erik,
>>>>>
>>>>> thanks for the reviews!
>>>>>
>>>>> Now I need a sponsor.
>>>>>
>>>>> Final webrev (same as before, including Reviewed-by line):
>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.05/ 
>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.05/>
>>>>>
>>>>> Thanks, Roman
>>>>>
>>>>>
>>>>>> Looks good Roman!
>>>>>>
>>>>>> /Per
>>>>>>
>>>>>> On 2017-11-06 12:13, Roman Kennke wrote:
>>>>>>> Am 26.10.2017 um 11:43 schrieb Per Liden:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 2017-10-25 18:11, Erik Osterlund wrote:
>>>>>>>> [...]
>>>>>>>>>> So let me just put your changes up for review (again), if you
>>>>>>>>>> don't
>>>>>>>>>> mind:
>>>>>>>>>>
>>>>>>>>>> Full webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/>
>>>>>>>> I like this. Just two naming suggestions:
>>>>>>>>
>>>>>>>> src/hotspot/share/gc/shared/gcArguments.hpp:
>>>>>>>>
>>>>>>>>    39   static jint create_instance();
>>>>>>>>    40   static bool is_initialized();
>>>>>>>>    41   static GCArguments* instance();
>>>>>>>>
>>>>>>>> Could we call these:
>>>>>>>>
>>>>>>>>    39   static jint initialize();
>>>>>>>>    40   static bool is_initialized();
>>>>>>>>    41   static GCArguments* arguments();
>>>>>>>>
>>>>>>>> Reasoning: initialize() maps better to its companion
>>>>>>>> is_initialized().
>>>>>>>> GCArguments::arguments() maps better to the existing patterns
>>>>>>>> we have
>>>>>>>> with CollectedHeap::heap().
>>>>>>> Ok, that sounds good to me. Actually, it's almost full-circle
>>>>>>> back to my
>>>>>>> original proposal ;-)
>>>>>>>
>>>>>>> Differential:
>>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04.diff/
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04.diff/>
>>>>>>> Full:
>>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04/
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04/>
>>>>>>>
>>>>>>> Ok to push now?
>>>>>>>
>>>>>>> Roman
>>>>>
>>>
>>