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

[Marcus Larsson]

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