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

[Jon Masamitsu]

On 04/27/2016 04:23 AM, Thomas Schatzl wrote:
> Hi again,
>
> On Wed, 2016-04-27 at 13:10 +0200, 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).
>>
>> - in AdaptiveSizePolicy::initial_numberOf_workers(), why is the lower
>> bound in line 358 two? I.e.
>>
>> initial_workers = MIN2(2U, ParallelGCThreads);
>>
>> - the indentation for the parameter list for
>> AdaptiveSizePolicy::add_workers() is always wrong :)
>>
>> - the changeset does not seem to apply cleanly to the latest hs tree.
>>
>> - 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.
>>
>> - 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.
>>
>> - 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.
>>
>> - 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.
>>
> Maybe rename the "initializing" parameter of add_workers() to
> "exit_on_failure", as this would concisely describe what it does
> without a long explanation.

I'm willing to make that change but it feels a bit odd to me.  The
way it is, I'll telling add_workers() that it is being called during
initialization (or not) and it is up to add_workers() to decide what
to do.  If I change the parameter to "exit_on_failure", I'm telling
add_workers() what to do on failure which means I have some
idea (as the caller) of what failure means to add_workers().

Jon
>
> Otherwise, in workgroup.hpp:166, the comment
> " 163     add_workers(false /* exit_on_failure */);" should be adapted.
>
> Thanks,
>    Thomas
>