RFR(s): 8068582: UseSerialGC not always set up properly

[Per Liden]

Hi Jon,

> On 20 Apr 2015, at 20:15, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:
> 
> Per,
> 
> In place of
> 
> It is not possible to combine the ParNew young collector with anything other than the CMS collector
> 
> let me suggest
> 
> It is not possible to combine the ParNew young collector with any collector other than CMS

Yep, that sounds better. Will fix.

> 
> Otherwise, still looks good.

Thanks Jon!

/Per

> 
> Jon
> 
> On 04/20/2015 01:28 AM, Per Liden wrote:
>> Hi Bengt, 
>> 
>> On 2015-04-20 09:45, Bengt Rutisson wrote: 
>>> 
>>> Hi Per, 
>>> 
>>> On 2015-04-17 16:05, Per Liden wrote: 
>>>> Hi, 
>>>> 
>>>> When no Use*GC flag is specified on the command-line we make a default 
>>>> GC selection. In case we select SerialGC we fail to set UseSerialGC to 
>>>> true (unlike when we select ParallelGC where we set UseParallelGC). 
>>>> This means we can run with all Use*GC set to false (which implicitly 
>>>> means we're using serial). It also means we can't use UseSerialGC as 
>>>> the sole indicator that we're using serial. 
>>>> 
>>>> This patch makes sure that we always set the corresponding Use*GC for 
>>>> the GC we have selected. 
>>>> 
>>>> A typical case where the VM selects serial but fails to set 
>>>> UseSerialGC is when running a client VM without specifying a Use*GC 
>>>> option on the command-line. 
>>>> 
>>>> Testing: Added a new jtreg test, passes jprt 
>>>> 
>>>> Webrev: http://cr.openjdk.java.net/~pliden/8068582/webrev.0/ <http://cr.openjdk.java.net/~pliden/8068582/webrev.0/> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8068582 <https://bugs.openjdk.java.net/browse/JDK-8068582> 
>>> 
>>> The changes look good, but there are a couple of comments in the code 
>>> that would be nice to clean up now that you enforce that UseSerialGC is 
>>> properly set  up: 
>>> 
>>> genCollectedHeap.hpp: 
>>> 
>>>    virtual bool can_elide_initializing_store_barrier(oop new_obj) { 
>>>      // We wanted to assert that:- 
>>>      // assert(UseSerialGC || UseConcMarkSweepGC, 
>>>      //       "Check can_elide_initializing_store_barrier() for this 
>>> collector"); 
>>>      // but unfortunately the flag UseSerialGC need not necessarily always 
>>>      // be set when DefNew+Tenured are being used. 
>>>      return is_in_young(new_obj); 
>>>    } 
>>> 
>>> 
>>> arguments.cpp: 
>>> 
>>>    if (UseParNewGC && !UseConcMarkSweepGC) { 
>>>      // !UseConcMarkSweepGC means that we are using serial old gc. 
>>> Unfortunately we don't 
>>>      // set up UseSerialGC properly, so that can't be used in the check 
>>> here. 
>>>      jio_fprintf(defaultStream::error_stream(), 
>>>          "It is not possible to combine the ParNew young collector with 
>>> the Serial old collector.\n"); 
>>>      return false; 
>>>    } 
>>> 
>>> 
>>> In both these cases I think we can just remove the comments. The 
>>> suggested assert in can_elide_initializing_store_barrier() is not 
>>> interesting to add in my opinion since it is code in GenCollectedHeap, 
>>> which we know is only used by Serial and CMS. 
>> 
>> Good catch. I will remove both of these. 
>> 
>> I'll also adjust the error message there from: 
>> 
>>  "It is not possible to combine the ParNew young collector with the Serial old collector. 
>> 
>> to: 
>> 
>> "It is not possible to combine the ParNew young collector with anything other than the CMS collector." 
>> 
>> Because it doesn't makes sense to mention SerialOld when you say e.g. -XX:+UseParNewGC -XX:+UseParallelOldGC 
>> 
>> Here's an updated webrev: 
>> http://cr.openjdk.java.net/~pliden/8068582/webrev.1/ <http://cr.openjdk.java.net/~pliden/8068582/webrev.1/> 
>> 
>> Diff against previous webrev: 
>> http://cr.openjdk.java.net/~pliden/8068582/webrev.1-diff/ <http://cr.openjdk.java.net/~pliden/8068582/webrev.1-diff/> 
>> 
>> Thanks for reviewing! 
>> 
>> /Per 
>> 
>>> 
>>> Thanks, 
>>> Bengt 
>>> 
>>>> 
>>>> /Per 
>>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150420/9aba6b5a/attachment.htm>