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

[Bengt Rutisson]

Thanks Jon and John for the reviews!

Just pushed this change.

Bengt


On 1/3/13 7:55 PM, Bengt Rutisson wrote:
>
> 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
>>>
>