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

[Kim Barrett]

On Oct 17, 2014, at 7:51 AM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
> 
>> ==============================================================================
>> 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.

The rewrite is better.

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

New version looks ok.

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

Ah, I was confused by the initial review request, which said
    "A recent change [1] forbidding this flag [kab: ParallelGCThread] to be set to 0 …”
but that recent change was G1-specific.

New version looks ok.  I’m not crazy about the really long line, but hotspot coding guidelines explicitly
disavow a line length limit.

> New webrev:
> http://cr.openjdk.java.net/~mlarsson/6979279/webrev.04/
> 
> Incremental:
> http://cr.openjdk.java.net/~mlarsson/6979279/webrev.03-04/

Looks good.