RFR: 8080106: Refactor setup of parallel GC threads

[Stefan Karlsson]

On 2015-05-15 04:53, Jon Masamitsu wrote:
>
>
> On 5/13/2015 6:17 AM, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review these patches to unify the ways we specify the number 
>> of used worker threads. The main goal for these patches is to get rid 
>> of CollectedHeap::set_par_threads() and CollectedHeap::n_par_threads().
>>
>> The RFE has been split into multiple sub-tasks:
>>  8080109: Use single-threaded code in 
>> Threads::possibly_parallel_oops_do when running with only one worker 
>> thread
>>  8080110: Remove usage of CollectedHeap::n_par_threads() from root 
>> processing
>>  8080111: Remove SubTaskDone::_n_threads
>>  8080112: Replace and remove the last usages of 
>> CollectedHeap::n_par_threads()
>>  8080113: Remove CollectedHeap::set_par_threads()
>>
>>
>>
>> ---
>> http://cr.openjdk.java.net/~stefank/8080110/webrev.00
>> 8080110: Remove usage of CollectedHeap::n_par_threads() from root 
>> processing
>>
>> Remove the explicit usages of CollectedHeap::n_par_threads() from the 
>> root processing code.
>>
>> The proposal is to pass the number of worker threads via the 
>> StrongRootsScope object. The StrongRootsScope object is already setup 
>> from single-threaded code where we know how many worker threads are 
>> going to be used.
>>
>> ---
> http://cr.openjdk.java.net/~stefank/8080110/webrev.00/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp.frames.html 
>
>
> 2925   CMSParInitialMarkTask(CMSCollector* collector, 
> StrongRootsScope* strong_roots_scope, uint n_workers) :
> 2926       CMSParMarkTask("Scan roots and young gen for initial mark 
> in parallel", collector, n_workers),
> 2927       _strong_roots_scope(strong_roots_scope) {}
>
>
> CMSParInitialMarkTask and CMSParRemarkTask constructors both take a 
> StrongRootsScope* (which has the number of
> workers) and a n_workers.  Did you look at whether the n_workers 
> parameter was needed?  If this is just one
> change too many to make here, that's fine.

Yes, I have changed that in another patch that I'll hopefully get to 
after this set of patches.


>
> http://cr.openjdk.java.net/~stefank/8080110/webrev.00/src/share/vm/gc_implementation/g1/g1RootProcessor.cpp.udiff.html 
>
>
> Is set_num_workers() needed?
>
>  void G1RootProcessor::set_num_workers(uint active_workers) {
> +  assert(active_workers == _srs.n_threads(),
> +      err_msg("Mismatch between number of worker threads. 
> active_workers: %u and n_workers(): %u",
> +              active_workers,
> +              _srs.n_threads()));
>    _process_strong_tasks->set_n_threads(active_workers);
>  }
>
> You already have "active_workers" from the StrongRootsScope.

As you noted in your other mail, this is removed in one of the later 
patches.

>
> http://cr.openjdk.java.net/~stefank/8080110/webrev.00/src/share/vm/memory/defNewGeneration.cpp.udiff.html 
>
>
> +  {
> +    // SerialGC runs with n_workers == 0.
> +    StrongRootsScope srs(0);
>
> I would have thought 1 would be passed instead of 0.  The 80801009 
> change uses 1 for serial, right?

I would prefer to have it set to 1, but that is currently causing 
problems with DefNew.

It boils down to this code:
void CardTableModRefBS::non_clean_card_iterate_possibly_parallel(Space* sp,
MemRegion mr,
OopsInGenClosure* cl,
CardTableRS* ct,
uint n_threads) {
   if (!mr.is_empty()) {
     if (n_threads > 0) {
#if INCLUDE_ALL_GCS
       non_clean_card_iterate_parallel_work(sp, mr, cl, ct, n_threads);
#else  // INCLUDE_ALL_GCS
       fatal("Parallel gc not supported here.");
#endif // INCLUDE_ALL_GCS
     } else {
       // clear_cl finds contiguous dirty ranges of cards to process and 
clear.

       DirtyCardToOopClosure* dcto_cl = sp->new_dcto_cl(cl, precision(), 
cl->gen_boundary(), false);
       ClearNoncleanCardWrapper clear_cl(dcto_cl, ct, false);

       clear_cl.do_MemRegion(mr);
     }
   }
}

If we try to run with StrongRootScope srs(1) we get n_threads == 0 and 
call non_clean_card_iterate_parallel_work, which is causing crashes. And 
if we build without INCLUDE_ALL_GCS we end up hitting the fatal(...) call.

Maybe we should consider changing  if (n_threads > 0) to if (n_threads > 
1) and also use the the single-threaded code here, just like the change 
for Threads::possibly_parallel_oops_do?

>
> That's all.  The rest looks good.

Thanks for reviewing.

StefanK

>
> Jon
>
>