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

[Jon Masamitsu]

On 04/27/2016 09:31 AM, Thomas Schatzl wrote:
> Hi,
>
> On Wed, 2016-04-27 at 09:04 -0700, Jon Masamitsu wrote:
>> 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.
> I would start up with one thread.

Hearing no objects, I will make it so.

>
>>> - 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?
> No, the intention has been fine, aligned right below the first
> parameter.
>
> In the webrev and the code I applied the patch the parameters (in the
> hpp file) are aligned below the "e" of add_workers.
>
> It's odd because in the actual patch it is fine. Ignore this then,
> sorry for the noise.
>
>>> - 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's fine with me.

I created

https://bugs.openjdk.java.net/browse/JDK-8155263

But I also noticed that ParallelGC did not use DisableStartThread.
Since ParallelGC and the other guys are sharing this code,
I will be changing behavior one way or the other so I have
deleted the use in adaptiveSizePolicy.inline.hpp.

>>> - 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.
> That's just a suggestion. If you do not like it, ignore this comment.

I do not line inout parameters so will try to clean it up.  Also, I think
the return value of add_workers() is not really used up stream so
that will allow me some flexibility if true.

Jon

>
> Thomas
>