RFR: 8080106: Refactor setup of parallel GC threads

[Kim Barrett]

On May 18, 2015, at 7:17 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> On 2015-05-14 05:01, Kim Barrett wrote:
>> ==============================================================================
>> 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.

Thanks, that confirms what I’d puzzled out.

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

I’m not surprised you don’t understand, since I don’t either.  I think I missed that different returnChunkXXX functions were being called.

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

Yes, as that presumably involves TLS access.  Also, is_GC_task_thread is a virtual function, where n_par_threads is just a data member access.

I don’t know if the difference is noticeable, in amongst all the other stuff going on.  But this looks to me like it might be a pretty common code path.  And no, I don’t have a better suggestion right now.  Knowing the appropriate value at some high level in the call chain and passing it down through all the relevant calls looks like it would be pretty ugly.

Although it seems that for the n_par_threads-based version to work and produce the same result, there must be some global context switch between the is_par and !is_par state.  Is there some other bit in the heap that corresponds to that context?  E.g. do GC-task calls into here only occur during pauses and other calls only during non-pauses, or something like that?  But then there might be a single-threaded pause case?  Sorry, I don’t know the relevant code, so might be just making things up.

>> ------------------------------------------------------------------------------
>> 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!”);

Shouldn’t that be “>” in the error message?