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

[Bengt Rutisson]

Hi Per,

On 2015-04-20 10:28, 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/

Looks good! Thanks for fixing these things too!

Bengt

>
> Thanks for reviewing!
>
> /Per
>
>>
>> Thanks,
>> Bengt
>>
>>>
>>> /Per
>>