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