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

[Jon Masamitsu]

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

Otherwise, still looks good.

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/
>>> Bug: 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/
>
> Diff against previous webrev:
> 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/41bb3218/attachment.htm>