Request for review (S): 8005396: Use ParNew with only one thread instead of DefNew as default for CMS on single CPU machines

[Bengt Rutisson]

Hi Jon,

On 12/31/12 4:18 PM, Jon Masamitsu wrote:
> Bengt,
>
> Thanks for the changes.

Thanks for looking at it again!

> Could you also fix this code in
>
> share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.cpp 
>
>
> If CMS is never going to see code with ParallelGCThreads == 0, then
> the then-block for 624 can be deleted.

I think it is a little early to remove this code. My change only makes 
sure that if you run CMS with ParNew you get at least one worker thread 
to make sure that you can use ParNew.

If you explicitly turn ParNew off you will still get DefNew and can use 
ParallelGCThreads == 0. My other review request will print a deprecation 
message for this combination but for JDK8 it will still be allowed.

For JDK9 we plan to completely disallow this combination and at that 
point I think we can remove the code in compactibleFreeListSpace.cpp.

Thanks,
Bengt

>
>
>    620        if (!_adaptive_freelists&& _smallLinearAllocBlock._ptr 
> == NULL) {
>
>    621          // Mark the boundary of the new block in BOT
>
>    622          _bt.mark_block(prevEnd, value);
>
>    623          // put it all in the linAB
>
>    624          if (ParallelGCThreads == 0) {
>
>    625            _smallLinearAllocBlock._ptr = prevEnd;
>
>    626            _smallLinearAllocBlock._word_size = newFcSize;
>
>    627 repairLinearAllocBlock(&_smallLinearAllocBlock);
>
>    628          } else { // ParallelGCThreads>  0
>
>    629            MutexLockerEx x(parDictionaryAllocLock(),
>
>    630 Mutex::_no_safepoint_check_flag);
>
>    631            _smallLinearAllocBlock._ptr = prevEnd;
>
>    632            _smallLinearAllocBlock._word_size = newFcSize;
>
>    633 repairLinearAllocBlock(&_smallLinearAllocBlock);
>
>    634          }
>
>
> Jon
>
> On 12/30/12 00:11, Bengt Rutisson wrote:
>>
>> Hi John and Jon,
>>
>> Thanks for the reviews!
>>
>> I discovered a bug in my fix. ParNew actually does not support 
>> ParallelGCThreads=0. I fixed this by making sure that we don't set 
>> ParallelGCThreads to 0 on single CPU machines. Instead we keep it at 1.
>>
>> And if someone explicitly set -XX:ParallelGCThreads=0 on the command 
>> line while trying to use ParNew I print an error message and exit.
>>
>> I assume that this is the reason that we previously picked DefNew if 
>> ParallelGCThreads was set to 0, but I think now that we want to 
>> deprecate DefNew for CMS it makes more sense to require users to 
>> explicitly turn ParNew off with -XX:-UseParNewGC if this is what they 
>> really want.
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~brutisso/8005396/webrev.02/
>>
>> The only change to the previous version is in arguments.cpp. Here is 
>> the small diff compared to the previous webrev:
>> http://cr.openjdk.java.net/~brutisso/8005396/webrev.01-02.diff/
>>
>> I have tested the fix on a single CPU virtual box instance.
>>
>> Thanks,
>> Bengt
>>
>> On 12/22/12 1:12 AM, John Coomes wrote:
>>> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
>>>> Hi All,
>>>>
>>>> Can I have a couple of reviews for this change?
>>>>
>>>> http://cr.openjdk.java.net/~brutisso/8005396/webrev.00/
>>>>
>>>> Currently we use ParNew as default for the young generation when 
>>>> CMS is
>>>> selected. But if the machine only has a single CPU we set the
>>>> ParallelGCThreads to 0 and and select DefNew instead of ParNew.
>>> Looks good to me.
>>>
>>> -John
>>>
>>>> As part of another change, 8003820, we will deprecate the DefNew + CMS
>>>> combination. Thus, it does not make sense anymore to have this 
>>>> selected
>>>> by default. This fix is to make CMS always pick ParNew by default.
>>>>
>>>> The change also has the side effect that the, in my opinion, rather
>>>> strange behavior that setting ParallelGCThreads=0 on the command line
>>>> overrides the GC choice. I would expect this command line to give me
>>>> ParNew, but it actually gives me DefNew:
>>>>
>>>> -XX:+UseParNewGC -XX:ParallelGCThreads=0
>>>>
>>>> After my proposed change you get ParNew with the above command line.
>>>>
>>>> I have done some performance testing to verify that ParNew with one
>>>> thread is not slower than DefNew. The details are in the bug report:
>>>>
>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8005396
>>>>
>>>> but as a summary it can be said that there is no noticeable 
>>>> difference.
>>>>
>>>> I am also running some more SPECjbb2005 runs and will analyze the 
>>>> gc times.
>>>>
>>>> Thanks,
>>>> Bengt
>>