RFR: 8080106: Refactor setup of parallel GC threads

[Kim Barrett]

On May 19, 2015, at 8:30 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> On 2015-05-18 23:21, Kim Barrett wrote:
>>>> ------------------------------------------------------------------------------
>>>> 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.
>> […]
> 
> I added code to splitChunkAndReturnRemainder to count the number of times we execute the new Thread::current()->is_GC_task_thread() call. Out of the benchmarks in gc-test-suite, I see that GCBasher is the benchmark that causes the JVM to execute this path the most. In average the JVM enters this path about 100 times per GC, which I think is a fairly low number compared to everything else we're doing during the GCs.
> 
> […]
> Do you want me to do any further experiments with this?

Thanks for the measurements.  That’s enough to convince me the change is ok.

>>>> ------------------------------------------------------------------------------
>>>> 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?
> 
> I think it depends on how you interpret "# requested". I think I'll change the assert message to be err_msg("n_threads: %u > ParallelGCThreads: %u", ...), or something like that.

I see the ambiguity you are suggesting.  Changing in the described way would make it much better.