RFR: 8080106: Refactor setup of parallel GC threads

[Stefan Karlsson]

Hi Kim,

On 2015-05-14 05:01, Kim Barrett wrote:
> On May 13, 2015, at 9:17 AM, Stefan Karlsson <stefan.karlsson at oracle.com> 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()
>>
>> See the description below for each individual patch:
> Thanks for breaking this up into multiple webrevs.
>
> I'm only part way through; I haven't looked at the webrev for 8080113
> yet.  Here's what I've got so far.
>
> ==============================================================================
> http://cr.openjdk.java.net/~stefank/8080109/webrev.00
> 8080109: Use single-threaded code in Threads::possibly_parallel_oops_do when running with only one worker thread
>
> Looks good.
>
> ==============================================================================
> http://cr.openjdk.java.net/~stefank/8080110/webrev.00
> 8080110: Remove usage of CollectedHeap::n_par_threads() from root processing
>
> I would have found this easier to review if the description contained
> some discussion of the elimination of MarkScope::_active.

Yes. I actually intended to move the setup of StrongRootScope and the 
removal of MarkScope::_active as a separate patch, but that patch was 
hard to motivate without the rest of 8080110.

MarkScope::_active was used to allow the single-threaded code to call 
gen_process_roots without having to create explicitly create s 
StrongRootsScope before calling gen_process_roots. With this patch all 
callers of gen_process_roots have to pass a pointer to a 
StrongRootsScope as an argument to gen_process_roots, so there's no need 
to keep the MarkSweep::_active variable anymore.

>
> ------------------------------------------------------------------------------
> src/share/vm/memory/genCollectedHeap.hpp
>   401   void gen_process_roots(StrongRootsScope* scpe,
>
> scpe => scope

Fixed.

>
> ------------------------------------------------------------------------------
> src/share/vm/memory/strongRootsScope.hpp
>
> Pre-existing defect: For the MarkScope class, at least the destructor
> should be protected, to ensure no accidental slicing.  The constructor
> can also be protected.

I changed it to protected.

>
> ==============================================================================
> http://cr.openjdk.java.net/~stefank/8080111/webrev.00
> 8080111: Remove SubTaskDone::_n_threads
>
> Looks good.

Thanks.

>
> ==============================================================================
> http://cr.openjdk.java.net/~stefank/8080112/webrev.00
> 8080112: Replace and remove the last usages of CollectedHeap::n_par_threads()
>
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.cpp
> 1899   if (rem_sz < SmallForDictionary) {
> 1900     // The freeList lock is held, but multiple GC task threads might be executing in parallel.
> 1901     bool is_par = Thread::current()->is_GC_task_thread();
> 1902     if (is_par) _indexedFreeListParLocks[rem_sz]->lock();
> 1903     returnChunkToFreeList(ffc);
> 1904     split(size, rem_sz);
> 1905     if (is_par) _indexedFreeListParLocks[rem_sz]->unlock();
> 1906   } else {
> 1907     returnChunkToDictionary(ffc);
> 1908     split(size, rem_sz);
> 1909   }
>
> Conditional locking could be eliminated by changing the test on line
> 1899 to also test the is_par expression.  OTOH, see next comment.

One branch calls returnChunkToFreeList while the other calls 
returnChunkToDictionary, so I'm not sure I understand what you suggest.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.cpp
> 1900     bool is_par = (GenCollectedHeap::heap()->n_par_threads() > 0);
> =>
> 1901     bool is_par = Thread::current()->is_GC_task_thread();
>
> This new mechanism for computing is_par seems like it might be
> substantially more expensive than the old mechanism.  I'm not sure
> whether it's likely to be noticably so.

Do you mean the call to Thead::current()? Do you have any suggestion on 
how to rewrite this?

>
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/g1/concurrentMark.cpp
> 1998   uint n_workers;
> ...
> 2004   n_workers = g1h->n_par_threads();
> 2005   assert(g1h->n_par_threads() == n_workers,
> 2006          "Should not have been reset");
> =>
> 2002   uint n_workers = _g1h->workers()->active_workers();
>
> Nice!

Thanks.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/parNew/parCardTableModRefBS.cpp
>    44   assert(n_threads <= (uint)ParallelGCThreads, "# worker threads != # requested!");
>
> Pre-existing defect: The error message says "!=" but the test is "<=".

Changed to:
   assert(n_threads <= (uint)ParallelGCThreads, "# worker threads < # 
requested!");

>
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/parNew/parCardTableModRefBS.cpp
>    61   bool parallel = n_threads > 0;
>
> There's an assertion on line 43 that n_threads > 0.

You're right! I removed the 'parallel' parameter from process_stride.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/parNew/parCardTableModRefBS.cpp
>    65     process_stride(sp, mr, stride, n_strides,
>    66                        parallel,
>    67                    cl, ct,
>
> Strange indentation of arguments.

I've removed the 'parallel' parameter.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc_interface/collectedHeap.hpp
>   294   virtual void set_par_threads(uint t) { (void)t; };
>
> The "standard" way to ignore a parameter is to not name it, e.g.
>
>    virtual void set_par_threads(uint /* t */) { }

OK. This code is removed in the next patch.

>
> Pre-existing defect: The trailing semi-colon isn't needed.

OK.

Thanks for reviewing!

StefanK

>
> ==============================================================================
>