RFR: 8079275: Remove CollectedHeap::use_parallel_gc_threads

[Stefan Karlsson]

Hi Kim,

On 2015-05-04 17:48, Kim Barrett wrote:
> On May 4, 2015, at 9:34 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>> Hi,
>>
>> Please review this patch to remove CollectedHeap::use_parallel_gc_threads.
>>
>> http://cr.openjdk.java.net/~stefank/8079275/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8079275
>>
>> This function is used from:
>>
>> 1) CMS - CMS can't be started with -XX:ParallelGCThreads=0 anymore, so all checks against use_parallel_gc_threads will always be true.
>>
>> 2) GenCollectedHeap::process_roots - This usage could be replaced with n_par_threads(), the same check the other parallel task (Threads::possibly_parallel_oops_do) uses.
>>
>> This will change the executed code for CMS but not Serial.
>>
>> Serial will always return false for CollectedHeap::use_parallel_gc_threads and GenCollectedHeap:n_par_threads(). See:
>> void GenCollectedHeap::set_par_threads(uint t) {
>>   assert(t == 0 || !UseSerialGC, "Cannot have parallel threads");
>>   CollectedHeap::set_par_threads(t);
>>
>> CMS will always return true for CollectedHeap::use_parallel_gc_threads, but can now return either true or false for n_par_threads() depending on how n_par_threads was setup for the currently executing task. This means that some CMS paths will now call StringTable::oops_do instead of StringTable::possibly_parallel_oops_do, when running single threaded.
>>
>> Thanks,
>> StefanK
> Nice cleanup.
>
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
>   229   typedef CMSParGCThreadState* CMSParGCThreadStatePtr;
>
> The change to eliminate the if-block changed the scope of this
> typedef, extending it to the end of the function.  Is this typedef
> actually needed?  I don't see any obvious reason why the use of it in
> the NEW_C_HEAP_ARRAY call couldn't just use the pointer type directly,
> rather than needing a typedef.
>
> If the typedef doesn't have some useful purpose in the
> NEW_C_HEAP_ARRAY call, then I'd rather see it removed, especially now
> with the scope extension.  Your call.  I don't need a new webrev just
> for this change, if you decide to make it.
>
> [And I was about to send this when StefanJ's similar suggestion
> arrived in my in-box.  So two votes for removal.]
>
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
> 2133   ModUnionClosure* muc = CollectedHeap::use_parallel_gc_threads() ?
> 2134                                                &_modUnionClosurePar
> 2135                                                : &_modUnionClosure;
> 2136   _cmsGen->gc_prologue_work(full, registerClosure, muc);
> =>
> 2130   _cmsGen->gc_prologue_work(full, registerClosure, &_modUnionClosurePar);
>
> It looks like this change eliminated the only reference to _modUnionClosure.
>
> ------------------------------------------------------------------------------

Both issues have been addressed in webrev.02.

Thanks,
StefanK