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

Bengt Rutisson bengt.rutisson at oracle.com
Wed Oct 15 12:44:12 UTC 2014


Hi Marcus,

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

Latest webrev looks good.

Bengt

>
> 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




More information about the hotspot-gc-dev mailing list