Request for Review (M) - 6858051: Create GC worker threads dynamically

[Jon Masamitsu]

Thomas,

Thanks for looking at this.

On 04/27/2016 04:10 AM, Thomas Schatzl wrote:
> Hi Jon,
>
> On Tue, 2016-04-26 at 22:06 -0700, Jon Masamitsu wrote:
>> 6858051: Create GC worker threads dynamically
>> https://bugs.openjdk.java.net/browse/JDK-6858051
>>
>> This change creates a minimal number of GC workers at JVM
>> initialization and then adds additional workers as they are
>> needed.  This has been made part of the UseDynamicNumberOfGCThreads
>> feature.  When a parallel task is about to be executed, the number of
>> workers needed to execute the task is calculated (call that number
>> N).  If that number of workers already exist, then no additional
>> workers are created.  If fewer than N exists, then additional workers
>> are created.  Workers are not destroyed if not needed for the task.
>>
>> The UseParallelGC way of executing parallel tasks is different from
>> the other collectors so it has a somewhat separate implementation.
>>
>> http://cr.openjdk.java.net/~jmasa/6858051/webrev.00/
>>
>> Testing: gc_test_suite with and without UseDynamicNumberOfGCThreads.
>> Performance testing with the prototype did not show any regression
>> with UseDynamicNumberOfGCThreads turned on.     RBT testing for
>> hotspot_gc is in progress.
>    I only looked briefly at the change, but looks good so far.
>
> Some questions:
>
> - wouldn't it be useful (as part of a different change maybe) to always
> dynamically create the worker threads? I mean, always start up with a
> single gc thread and take the hit on the first time the threads are
> used instead? (Or maybe just make UseDynamicNumberOfGCThreads=true
> default).

To preserve current behavior I'd would not always create the workers
on demand.  For one thing if we can't create the workers at initialization,
I think we exit.  That might be preferable to some people as opposed to
limping along with a few workers.  Also the first GC would take a hit and
we'd probably have to implement pre-touch like flag 
CreateGCWorkersAtStartUp.

I'd prefer to get to the point where we can turn UseDynamicNumberOfGCThreads
on by default.
>
> - in AdaptiveSizePolicy::initial_numberOf_workers(), why is the lower
> bound in line 358 two? I.e.
>
> initial_workers = MIN2(2U, ParallelGCThreads);

That's the minimum I thought would make sense.   The amount of used in
the heap (one metric for deciding how many GC threads to use) is at a value
that doesn't reflect much about the application so is not useful.  I 
could start
with something else if there is data that will point me in the right 
direction.

> - the indentation for the parameter list for
> AdaptiveSizePolicy::add_workers() is always wrong :)

   return AdaptiveSizePolicy::add_workers(this,

                                          _active_workers,

                                          (uint) _workers,

                                          _created_workers,

                                          worker_type,

                                          initializing);


What's the rule now?  All on one line?

>
> - the changeset does not seem to apply cleanly to the latest hs tree.

I woke up in the middle of the night and realized that :-).  I fixed it 
earlier
today.

>
> - I would prefer if the dependencies on AdaptiveSizePolicy to WorkGang
> (at least) were minimized. The initial_number_of_workers() result could
> be passed in to AbstractWorkGang::initialize_workers() instead of
> adding the dependency.
Ok.  I'll make that change.

>
> - the AdaptiveSizePolicy::add_workers() method seems to be completely
> unrelated to that class. I.e. instead of doing policy stuff, it
> actually does "real work".
> I would prefer if that method were put into a separate class in the
> shared directory.

Ok.
>
> - the use of DisableStartThread in add_workers() seems odd. From the
> description it only seems to be applicable to Java threads, not native
> threads we are creating here.

What you say is true and that might be a bug.  I use it in add_workers() to
get the same effect as with the original code where all the workers are
started at initialization.  I probably should guard its use under the
"initializing" flag.   Would it be Ok if I remove its use for GC threads in
a separate change?

>
> - that is kind of my personal preference, but I would prefer if the
> interface to add_workers were a bit simpler - mainly not having that
> many inout parameters. Like "creates X new threads for the given
> holder, starting from index Y and returns the number of created
> threads."
>
> Then again, you would have some code duplication in the callers.

I'll make some changes and see if I get what you're saying.

Jon

>
> Will continue looking.
>
> Thanks,
>    Thomas
>
>