RFR: 8080106: Refactor setup of parallel GC threads

[Jon Masamitsu]

On 5/18/2015 4:58 AM, Stefan Karlsson wrote:
> 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 has appeal (using n_threads > 1) but it seems like that would require
SerialGC code to include threads support.

I see the difficulty and why you continued with n_threads > 0. I'm
ok with that because I think more clean up in this area is coming which
may make the right choice more clear.

Still all looks good.

Jon
>
>>
>> That's all.  The rest looks good.
>
> Thanks for reviewing.
>
> StefanK
>
>>
>> Jon
>>
>>
>