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

[Marcus Larsson]

Hi,

Sorry for the noise. Forgot to fix some additional cases where checks if 
ParallelGCThreads > 0 could be removed.

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

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

Thanks,
Marcus

On 15/10/14 13:37, Marcus Larsson wrote:
> 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