RFR: 6979279: remove special-case code for ParallelGCThreads==0

[Marcus Larsson]

Hi again,

Found a small issue with the last change. When running with CMS+DefNew 
the following assert fails in share/vm/memory/freelist.cpp in the 
assert_proper_lock_protection_work() function:

294:  assert(ParallelGCThreads > 0, "Don't call this directly");

The assert is no longer valid since locks will now be used regardless of 
ParallelGCThreads count, so the assert can safely be removed. I also 
removed some duplicate code in AdaptiveFreeList and merged the 
assert_proper_lock_protection and its _work function, removing the need 
for an additional assert as well.

New webrev:
http://cr.openjdk.java.net/~mlarsson/6979279/webrev.02/

Incremental:
http://cr.openjdk.java.net/~mlarsson/6979279/webrev.01-02/

Thanks,
Marcus

On 09/10/14 14:35, Jesper Wilhelmsson wrote:
> Yes, this looks really nice! Thanks for removing the non-parallel case!
> Ship it!
> /Jesper
>
>
> Marcus Larsson skrev 9/10/14 14:22:
>> Hi Jesper,
>>
>>
>> On 08/10/14 15:44, Jesper Wilhelmsson wrote:
>>> Hi,
>>>
>>> Looks good in general. Two comments:
>>>
>>> * In compactibleFreeListSpace.cpp the code:
>>>
>>>   630           MutexLockerEx x(parDictionaryAllocLock(),
>>>   631                           Mutex::_no_safepoint_check_flag);
>>>
>>> was previously executed for all values of ParallelGCThreads > 0, now it
>>> is not executed when ParallelGCThreads = 1. Seems legit, just want to
>>> make sure this is intentional. Are there any side effects to creating
>>> this lock that is expected to happen for PGCT = 1?
>>>
>>
>> Rather than making sure this was the case, I figured I might as well
>> remove the
>> special handling of the non-parallel case altogether, like the bug
>> suggests. The
>> only time this special case is used is if CMS+DefNew is used, which is a
>> deprecated combination.
>>
>> I also took the opportunity and changed a few other similar cases
>> where some
>> locks were avoided if PGCT==0.
>>
>>> * In g1HotCardCache you remove the variable _hot_cache_par_chunk_size
>>> and read directly from the flag in G1HotCardCache::drain() . I would
>>> prefer to keep the variable and only read from the flag during
>>> initialization. I think it would be nice if we were moving towards not
>>> reading directly from command line flags during GC. There have been a
>>> few cases lately where we have been in need of changing settings for the
>>> GC at runtime (when making flags manageable for instance) and I think we
>>> will see more of this in the future if we want to see a GC that can
>>> behave differently in different situations. It is a lot easier to do
>>> this if the flags can be changed at any time without needing to worry
>>> about them being read during GC.
>>>
>>
>> Fixed!
>>
>>> Thanks!
>>> /Jesper
>>>
>>
>>
>> Additionally, I noticed that I had previously missed the fact that
>> ConcurrentMark::use_parallel_marking_threads() will now always be
>> true, so the
>> updated change also removes that function and its uses.
>>
>> New webrev:
>> http://cr.openjdk.java.net/~mlarsson/6979279/webrev.01/
>>
>> Incremental:
>> http://cr.openjdk.java.net/~mlarsson/6979279/webrev.00-01/
>>
>> Thanks,
>> Marcus