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

Bengt Rutisson bengt.rutisson at oracle.com
Tue Oct 21 11:31:13 UTC 2014


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




More information about the hotspot-gc-dev mailing list