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

[Jon Masamitsu]

Bengt,

Thanks for the changes.  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.

    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
>