RFR: 8080106: Refactor setup of parallel GC threads

[Kim Barrett]

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.

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

scpe => scope

------------------------------------------------------------------------------ 
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.

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

Looks good.

==============================================================================
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------ 
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!

------------------------------------------------------------------------------ 
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 "<=".

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------ 
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.

------------------------------------------------------------------------------
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 */) { }

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

==============================================================================