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

[Marcus Larsson]

Thanks for the reviews, Jesper, Bengt and Kim!

Marcus

On 21/10/14 13:31, Bengt Rutisson wrote:
>
> Hi Marcus,
>
> Webrev 04 looks good to me too.
>
> Bengt
>
> On 10/17/14 1:51 PM, Marcus Larsson wrote:
>> Hi Kim,
>>
>> On 16/10/14 19:32, Kim Barrett wrote:
>>> On Oct 15, 2014, at 8:31 AM, Marcus Larsson
>>> <marcus.larsson at oracle.com> wrote:
>>>>
>>>> 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/
>>>
>>> ==============================================================================
>>>
>>> g1/g1HardCardCache.hpp - no changes?
>>>
>>
>> Not sure why webrev still includes this file. It had changes in a
>> previous revision of the change, but i removed/reverted these changes
>> after Jesper's review.
>>
>>> ==============================================================================
>>>
>>> g1/satbQueue.cpp
>>>   277 void SATBMarkQueueSet::set_closure(int i, ObjectClosure*
>>> closure) {
>>>   278   assert(_closures != NULL, "Precondition");
>>>   279   _closures[i] = closure;
>>>
>>> Perhaps add an assert that i is in proper range?  That seems like a
>>> more important check.
>>>
>>> Also, wouldn't it be better if i were size_t rather than int? Since
>>> you are changing the signature anyway.
>>>
>>
>> Good idea. Added the assert and changed the worker index type to uint.
>> (Made it uint rather than size_t since that seems to be standard for
>> the worker index.)
>>
>>> ==============================================================================
>>>
>>> g1/satbQueue.hpp
>>>   108   // Register "blk" as "the closure" for all queues.  Only one
>>> such closure
>>>   109   // is allowed.  The "apply_closure_to_completed_buffer"
>>> method will apply
>>>   110   // this closure to a completed buffer, and
>>> "iterate_closure_all_threads"
>>>   111   // applies it to partially-filled buffers (the latter should
>>> only be done
>>>   112   // with the world stopped).
>>>   113   void set_closure(int i, ObjectClosure* closure);
>>>
>>> I see no "blk" in the signature.  [Pre-existing problem.]
>>>
>>> The emphasis on "the closure" and "only one such closure" is left over
>>> from the non-parallel version, and seems like it should be updated.
>>>
>>
>> Rewrote the comment a bit.
>>
>>> ==============================================================================
>>>
>>> memory/freeList.hpp
>>> memory/freeList.cpp
>>> assert_proper_lock_protection()
>>>
>>> New implementation always involves a function call (to an empty
>>> implementation #ifndef ASSERT), rather than being optimizable to
>>> nothing #ifdef PRODUCT.  So I think the part of the change to this
>>> function around the use of PRODUCT/ASSERT is incorrect, and should be
>>> reverted.
>>>
>>
>> Yeah I realize this was not really optimal for product builds. Took
>> back the _work function, but only define and call it #ifdef ASSERT.
>> The function makes little sense if assertions are disabled.
>>
>>> ==============================================================================
>>>
>>> memory/sharedHeap.cpp
>>>    71   if ((UseParNewGC ||
>>>    72       (UseConcMarkSweepGC && (CMSParallelInitialMarkEnabled ||
>>>    73                               CMSParallelRemarkEnabled)) ||
>>>    74        UseG1GC) &&
>>>    75       use_parallel_gc_threads()) {
>>>
>>> Seems like we shouldn't need call to use_parallel_gc_threads() here.
>>>
>>
>> The call is still needed for the CMS part, although not for ParNew or
>> G1, so I moved it inside the appropriate parenthesis.
>>
>>> ==============================================================================
>>>
>>> gc_interface/collectedHeap.hpp
>>> [not in current webrev]
>>>
>>> Do we still need CollectedHeap::use_parallel_gc_threads() at all?
>>>
>>
>> The function is called in a few places where it still makes sense to
>> check it (the case above for example).
>>
>>> ==============================================================================
>>>
>>>
>>
>> Thanks for reviewing!
>>
>> New webrev:
>> http://cr.openjdk.java.net/~mlarsson/6979279/webrev.04/
>>
>> Incremental:
>> http://cr.openjdk.java.net/~mlarsson/6979279/webrev.03-04/
>>
>> Thanks,
>> Marcus
>